mirror of
https://github.com/Tria-plc/edr-platform.git
synced 2026-08-26 18:42:49 +00:00
458 lines
53 KiB
Markdown
458 lines
53 KiB
Markdown
# EDR Freight — Full QA / Logic Audit Report
|
||
|
||
**Date:** 2026-07-15
|
||
**Scope:** `@edr/freight-api` + freight backoffice/portal web apps
|
||
**Excluded per request:** warehouses, first-mile, last-mile, onboarding (Fayda/verifayda)
|
||
**Environment:** live stack via `pnpm run dev:freight`, API on `http://localhost:3030`, DB `edr_freight` @ `10.18.7.207`
|
||
**Method:** booted the real app, logged in as `superadmin@tria.com`, drove the API with `curl`, reproduced state bugs against live data, read every service/controller/entity in the in-scope modules, and cross-checked the backoffice screens against the API.
|
||
|
||
---
|
||
|
||
## 1. How to read this report
|
||
|
||
Every finding has: **Problem** (what is wrong + concrete failure), **Impact**, **Fix** (code-level, with `file:line`), and — where I ran it live — a **Repro** block with the actual request/response.
|
||
|
||
Severity:
|
||
|
||
- **CRITICAL** — money moves wrongly, or anyone can act on anyone's data / settle invoices.
|
||
- **HIGH** — data corruption, cross-tenant read/write, physical-asset state diverges from reality.
|
||
- **MEDIUM** — wrong-but-recoverable state, missing guards, math/notification errors.
|
||
- **LOW** — hardening, stale displays, latent (unused endpoint) bugs.
|
||
|
||
**Counts:** 6 Critical · 18 High · 27 Medium · 20 Low (≈71 distinct issues).
|
||
|
||
The single most valuable structural fix is at the top of §4 — it eliminates a whole family of bugs.
|
||
|
||
---
|
||
|
||
## 2. Live tests I actually ran (evidence)
|
||
|
||
| # | Test | Result | Verdict |
|
||
|---|------|--------|---------|
|
||
| 1 | Login `superadmin@tria.com` | `success:true` + JWT | OK |
|
||
| 2 | Build train `TR-00002` in KALITY, 2 locomotives | created | OK |
|
||
| 3 | **Move coupled LOCO-004 to MOJO while train stays in KALITY** | `200`, train KALITY / loco MOJO | **BUG — your example, confirmed** |
|
||
| 4 | Re-use same locomotive on a 2nd train | `409 already coupled` | Guard OK |
|
||
| 5 | `PATCH /train-builder/:id/yard` → MOJO | coupled locos + wagons follow | OK (builder path correct) |
|
||
| 6 | Decommission a coupled locomotive | `200 OUT_OF_SERVICE` | **BUG — no coupling guard** |
|
||
| 7 | Attach KALITY wagon to MOJO train | `400 not in yard` | Guard OK |
|
||
| 8 | `PATCH /wagons/:id` yard → different yard while ASSIGNED | `200` accepted | **BUG** |
|
||
| 9 | `DELETE /wagons/:id` on a wagon coupled to a train | `200` + **row physically gone** | **BUG — hard delete, no guard** |
|
||
| 10 | `GET /payments/checkout` **no auth** | `200` | **BUG — public** |
|
||
| 11 | `POST /internal/payments/mark-paid` **no auth** | `400` (reached handler, not `401`) | **BUG — public** |
|
||
| 12 | `GET /payments/receipt/:id` **no auth** | `400` (reached handler) | **BUG — public** |
|
||
| 13 | `GET /files/:id` **no auth** | `404` (reached handler) | **BUG — public** |
|
||
| 14 | Control: `GET /locomotives`, `GET /incidents` no auth | `401` | Auth global guard works |
|
||
|
||
Tests 10–14 prove the "public" endpoints are genuinely reachable without a token (protected routes return `401`; these return `400`/`404`/`200` because they hit the handler).
|
||
|
||
> **Boot-time noise (not app bugs):** SMS + Email services fail on RabbitMQ `ACCESS_REFUSED` (broker creds), and Swagger warns about 3 duplicate DTO names (`UpdateProfileDto`, `RequestChangesDto`, `SignContractDto`) and a legacy `/api/*` route. See §9.
|
||
|
||
---
|
||
|
||
## 3. CRITICAL — money & authorization
|
||
|
||
### C1. Paying an invoice settles it instantly without any money moving
|
||
**File:** `edr-platform/apps/edr-freight-api/src/modules/billing/billing.service.ts:973-986`
|
||
A shipped "DEMO" shortcut fakes a `payment.succeeded` callback the moment a payment is initiated:
|
||
```ts
|
||
// DEMO: manually fire the gateway 'payment.succeeded' callback here …
|
||
if (!result.immediateSuccess) { await this.payment.handlePaymentEvent({ eventType: "payment.succeeded", … }) }
|
||
```
|
||
Real gateways return `REQUIRES_ACTION` (redirect), so `immediateSuccess` is false for essentially every payment → the invoice is marked **PAID**, the booking advances to `PAID`/batch allocation, and the clearance-fee gate opens. **A customer clicks "Pay", closes the page, pays nothing, and the freight ships.** If they *do* pay, the later real webhook is a no-op and money is collected against an already-settled invoice with no reconciliation.
|
||
**Fix:** delete the `if (!result.immediateSuccess)` block. Settle only from `settleByPaymentId` on a verified provider signal, and check `intent.amountMinor` against the invoice balance at settle time.
|
||
|
||
### C2. Unauthenticated endpoints can settle any invoice by ID *(live-confirmed)*
|
||
**Files:** `billing/payment.controller.ts:55-104` (`GET /payments/checkout`, `@Public()`), `payment/internal-payment.controller.ts:22-39` (`POST /internal/payments/mark-paid`, `@Public()` — its own comment: *"anyone who can reach the API can mark payments as paid"*), `billing/payment.controller.ts:39-53` (`POST /payments/initiate`, no guard, no ownership check).
|
||
**Repro (live):**
|
||
```
|
||
GET /api/payments/checkout?invoiceId=…&method=TELEBIRR → 200 (no token)
|
||
POST /api/internal/payments/mark-paid → 400 (no token, reached handler)
|
||
GET /api/locomotives → 401 (control)
|
||
```
|
||
Combined with **C1**, anyone with an invoice UUID marks it paid unauthenticated and ships the freight. Anyone can POST a forged `payment.succeeded` (only a booking id is needed) to `mark-paid`.
|
||
**Fix:** shared-secret / `x-service-token` guard on `mark-paid` (the payment API already sends one — `http-payment-event-publisher.ts:44-45`); require auth + ownership on `initiate`; make `checkout` a signed, expiring URL.
|
||
|
||
### C3. Currency-unit chaos — providers disagree by 100× on the same charge
|
||
**Files:** `billing.service.ts:958` sends `amountMinor: Math.round(Number(invoice.balanceAmount))` (invoice amounts are **major** units, `numeric(14,2)`). Providers then split: `cbe-birr.provider.ts:57`, `ebirr.provider.ts:64`, `card.provider.ts:60`, `cac-bank.provider.ts:307` all do `amountMinor / 100`; but `telebirr.provider.ts:201`, `dmoney.provider.ts:200`, `waafi.provider.ts:282` treat it as major (no `/100`).
|
||
**Impact:** a 50,000 ETB invoice paid via CBE_BIRR/EBIRR/CARD/CAC charges **500.00** — while the freight side still marks it fully PAID (see C4). `Math.round` also drops cents.
|
||
**Fix:** pick one convention (`Math.round(balance*100)` true-minor everywhere, fix the 3 major-unit providers), then enforce `confirmedAmountMinor === intent.amountMinor` at settlement.
|
||
|
||
### C4. Settlement never verifies the amount charged
|
||
**Files:** `billing.service.ts:527-552` (`markInvoiceAsPaid` sets `paidAmount = totalAmount, balanceAmount = 0` purely from the invoice); `edr-payment-api/.../intents/intents.service.ts:408-415` (an amount mismatch is only `logger.error`'d — the intent still finalizes SUCCEEDED, and no freight handler even populates `confirmedAmountMinor`).
|
||
**Impact:** any wrong-amount success (C3's 1/100, a partial card capture, a reused stale intent) still marks the full invoice PAID.
|
||
**Fix:** treat a `confirmedAmountMinor` mismatch as a failure/hold; in `settleByPaymentId` refuse (or record a partial) when the confirmed amount doesn't cover `balanceAmount`.
|
||
|
||
### C5. Telebirr & D-Money webhook signatures are disabled
|
||
**Files:** `edr-payment-api/.../webhooks/handlers/telebirr-webhook.service.ts:16-18` (`const signatureValid = true;` + `// TODO: re-enable`), `dmoney-webhook.service.ts:16-17` (same). The pipeline only rejects when `signatureValid` is false, so these two are trusted unconditionally on the public `/webhooks/*` surface.
|
||
**Impact:** an attacker who guesses a `merch_order_id` POSTs a fake success payload → intent SUCCEEDED → freight marks the invoice PAID and ships.
|
||
**Fix:** implement `verifyWebhookSignature` for both; until keys exist, re-query the provider (`queryStatus`) before honoring SUCCEEDED.
|
||
|
||
### C6. Customers can create/act-on bookings billed to any company
|
||
**Files:** `bookings/bookings.service.ts:594-629` (`create` only resolves+verifies the caller's own company when `dto.companyId` is *absent*; a supplied `companyId` is used verbatim), plus a batch of booking mutations with **no ownership check** in `bookings.controller.ts`: `PATCH /:id` (205), `DELETE /:id` (585), `POST /:id/generate-price` (611), `/submit` (622), `/confirm-submit` (633), `/reject` (643), `/clearance/documents` (682), `/clearance/proceed` (699), `GET /:id/clearance` (672). Reads (`GET /:id`) *do* call `assertCustomerCanAccessBooking`; writes don't.
|
||
**Impact:** Company A's user sets `companyId` to Company B and books/invoices under B; or calls `DELETE /bookings/{B's id}` / submits / uploads clearance docs on B's booking. UUIDs appear in list payloads, so they're discoverable within a session.
|
||
**Fix:** require a staff permission to pass `companyId`; otherwise force it from the resolved user company. Add `assertCustomerCanAccessBooking` to every mutating booking route.
|
||
|
||
---
|
||
|
||
## 4. HIGH — Train / Locomotive / Wagon consist integrity
|
||
|
||
> **★ Structural root cause (fix this first).** The invariant *"a consist (train + its locomotives + its wagons) moves and locks as one unit"* is enforced **only inside** `trains/train-builder.service.ts`. Every *legacy / master-data* endpoint around it — `PATCH /locomotives/:id`, `PATCH /wagons/:id`, `POST /wagons/:id/assign-train`, `wagons/bulk-status`, `wagons/bulk-transfer`, `DELETE /trains/:id`, `DELETE /wagons/:id` — mutates the same rows with **no coupling guard**. Findings H1, H2, H5, H6, H7 are all the same missing check. **The one fix that kills the family:** make `TrainLocomotive`/`wagon.trainId` membership a guard that every locomotive/wagon mutation consults (reject or redirect to the builder endpoints).
|
||
|
||
### H1. ★ Locomotive yard/status freely editable while coupled to a built train — *your example, confirmed live*
|
||
**File:** `locomotives/locomotives.service.ts:87-119` (`update`).
|
||
`update()` applies `status` and `currentYardId` with **no check of the `train_locomotives` link table**:
|
||
```ts
|
||
currentYardId: dto.currentYardId === undefined ? locomotive.currentYardId : (dto.currentYardId ?? null),
|
||
```
|
||
The builder keeps train + locomotives in the same yard (`train-builder.service.ts:389-429 setYard`, `:656-660 validateAndLockLocomotives`), but `PATCH /locomotives/:id` bypasses it. **The frontend triggers it by accident:** the locomotive edit form always includes the `status` and `currentYardId` selects (`.../pages/fleet/config/resources.ts:168-169`) and PATCHes the *whole form object* on every save (`FleetResourcePage.tsx:301`). So editing a coupled locomotive's *name* re-sends its yard → the train and its locomotive end up in different yards.
|
||
**Repro (live):**
|
||
```
|
||
Build TR-00002 in KALITY with LOCO-004 (+LOCO-023)
|
||
PATCH /api/locomotives/LOCO-004 {"currentYardId": MOJO} → 200
|
||
GET train → yard: KALITY ; LOCO-004 → yard: MOJO ← diverged, no error
|
||
```
|
||
**Fix (exactly what you described — reject with a clear error, don't silently move):**
|
||
```ts
|
||
const link = await this.ds.getRepository(TrainLocomotive)
|
||
.findOne({ where: { locomotiveId: id }, relations: { train: true } });
|
||
if (link) {
|
||
if (dto.currentYardId !== undefined && dto.currentYardId !== link.train?.currentYardId)
|
||
throw new ConflictException(`Locomotive ${loco.code} is coupled to train ${link.train?.code}; move the train instead`);
|
||
if (dto.status !== undefined && dto.status !== loco.status)
|
||
throw new ConflictException(`Locomotive ${loco.code} is coupled to train ${link.train?.code}; detach it before changing status`);
|
||
}
|
||
```
|
||
Frontend: send only dirty fields, and disable the yard/status selects (show the modal error) when the locomotive is in a built train.
|
||
|
||
### H2. Wagon PATCH is a free-for-all — yard, `trainId`, `sequenceNumber`, `status` all unguarded *(live-confirmed)*
|
||
**File:** `wagons/wagons.service.ts:90-101` — `Object.assign(wagon, dto)` with zero invariant checks; `UpdateWagonDto` (PartialType of Create) exposes `trainId`, `sequenceNumber`, `status`, `currentYardId`.
|
||
**Repro (live):** `PATCH /api/wagons/:id {"currentYardId": MOJO}` on a wagon `ASSIGNED` to a KALITY train returned `200` and moved it. Setting `trainId` directly attaches to any train bypassing every builder rule; flipping `ASSIGNED→AVAILABLE` while `trainId` is set makes the wagon grabbable by transfer requests and the legacy assign flow.
|
||
**Fix:** reject `trainId`/`sequenceNumber` in update; when `wagon.trainId != null`, reject `currentYardId`/`status` changes (409 → point at train-builder endpoints).
|
||
|
||
### H3. `DELETE /wagons/:id` hard-deletes with no guard — destroyed real data during this audit *(live-confirmed)*
|
||
**File:** `wagons/wagons.service.ts:136-139` — `this.wagonRepo.remove(wagon)` is a **hard** delete (repo standard is soft-delete via `BaseEntity.deletedAt`). No check for `trainId`, live-schedule pinning, or containers.
|
||
**Repro (live):** I called `DELETE /api/wagons/{BW1-0009}` while it was `ASSIGNED` to my test train → `200`. Raw SQL then showed **the row physically gone** (`SELECT … WHERE id=… → 0 rows`). On delete, `train_set_wagons.physical_wagon_id` is `SET NULL` (a dispatched schedule silently loses its physical wagon) and `wagon_movements` is `CASCADE` (the audit ledger is destroyed).
|
||
> **I recreated BW1-0009 via the API** (`POST /api/wagons`, new UUID `d9b578ed-…`, AVAILABLE @ KALITY) so the dev data count is whole again. The original UUID `905cad62-…` is unrecoverable (hard delete). See §10.
|
||
**Fix:** block deletion when `trainId IS NOT NULL` or the wagon is pinned to a DRAFT/SCHEDULED/DISPATCHED schedule (reuse `train-builder`'s `isWagonPinnedToLiveSchedule`); switch to `softRemove` (add a partial unique index on `wagon_number WHERE deleted_at IS NULL`).
|
||
|
||
### H4. `DELETE /trains/:id` hard-deletes a built train and permanently strands its wagons
|
||
**File:** `trains/trains.service.ts:57-60` — `trainRepo.remove(train)`, no active-schedule check (contrast `train-builder.service.ts:536-563 disband`, which blocks on live schedules and releases resources). Wagons' FK is `SET NULL`, so they keep `status = ASSIGNED` with `trainId = null` → **unusable forever** (attach requires `status === Available`). The fleet Trains CRUD page (`resources.ts:204`) points at this endpoint.
|
||
**Fix:** delegate `TrainsService.remove` to `TrainBuilderService.disband` (block live schedules, reset wagons to Available, delete locomotive links), and use `softRemove`.
|
||
|
||
### H5. Legacy wagon assign: maintenance wagons assignable, silent theft, duplicate sequences
|
||
**File:** `wagons/wagons.service.ts:141-164` (`POST /wagons/:id/assign-train`). Only guard is `status === Assigned`. Holes vs the builder's `attachWagons`: a `MAINTENANCE`/`DETAINED` wagon is accepted and flipped to ASSIGNED (erasing the flag); a wagon with `trainId` set but status≠ASSIGNED has its `trainId` overwritten (**steals it from another train's consist**, never resequenced); no wagon-yard vs train-yard check; no `train.status !== IN_SERVICE` check; caller `sequenceNumber` isn't collision-checked (no unique index on `(train_id, sequence_number)`).
|
||
**Frontend echo:** `AssignWagonDialog.tsx:22-24` filters `w.status === Available || !w.trainId` — the `||` should be `&&`; today it offers maintenance/detained wagons.
|
||
**Fix:** re-implement on builder rules (`Available && trainId === null && sameYard && train not InService`, sequence = max+1), or delete the endpoint and use `POST /train-builder/:id/wagons`.
|
||
|
||
### H6. `bulk-status` / yard-workspace "Free up" corrupts wagons physically in a train
|
||
**File:** `wagons/wagons.service.ts:241-269` (`bulkSetStatus`, no `trainId` guard); UI `WagonYardWorkspaceModal.tsx:187,270-287,545-580` picks an arbitrary slice of ASSIGNED wagons and flips them AVAILABLE — including consist wagons (whose ASSIGNED means "coupled"). After the flip they still have `trainId` set but read AVAILABLE → transfer requests hand them out, `bulk-transfer` moves them, legacy assign re-homes them, while the builder consist still lists them.
|
||
**Fix:** refuse to change status of wagons with `trainId IS NOT NULL`; exclude them from the modal's flip pools.
|
||
|
||
### H7. `bulk-transfer` relocates train-coupled wagons; legacy unassign bypasses the schedule pin
|
||
**File:** `wagons/wagons.service.ts:180-234` (`bulkTransfer`, existence-only check → moves consist wagons to another yard) and `166-172` (`unassignFromTrain` frees a wagon with no `isWagonPinnedToLiveSchedule` check and leaves a sequence gap).
|
||
**Fix:** reject `trainId IS NOT NULL` in `bulkTransfer`; run the builder's pinned-schedule query in `unassignFromTrain` and resequence, or delete in favor of `DELETE /train-builder/:id/wagons/:wagonId`.
|
||
|
||
### H8. Export capacity check-then-act race → train overbooking
|
||
**File:** `bookings/booking-transition.service.ts:1093-1153` → `train-scheduling/booking-batch.service.ts:722-793` — `pickExportSchedule` reads `budget.fits(...)` then `reserve` (idempotency only, never re-checks capacity). No transaction/row-lock spans the check and the write (the code comment even calls the pre-check "rough").
|
||
**Impact:** two staff accept two export bookings for the same near-full train concurrently → both pass `fits()`, both reserve → train exceeds locomotive pull weight / wagon slots.
|
||
**Fix:** wrap check + reserve in a serializable transaction with `SELECT … FOR UPDATE` on the schedule (or a per-schedule advisory lock); re-verify `fits()` inside the lock.
|
||
|
||
### H9. Consolidation pairing race → one partner paired with two bookings
|
||
**File:** `bookings.service.ts:494-543` + `bookings.repository.ts:213-261,287-309` — `findComplementaryConsolidationPartner` filters `consolidationPartnerId IS NULL`, `pairConsolidation` writes both sides, no lock between find and pair.
|
||
**Impact:** two bookings both pick the same waiting partner → asymmetric pairing, shared-wagon capacity double-counted.
|
||
**Fix:** `FOR UPDATE` on the candidate inside a transaction, re-assert `consolidationPartnerId IS NULL` on both before writing.
|
||
|
||
### H10. `import-operations` controller has no authorization
|
||
**File:** `import-operations/import-operations.controller.ts` (whole file — only `@ApiTags`, no permission guard). Any authenticated portal user can `POST /import-operations/customs/{anyBookingId}/release-permitted`, assign customs risk, or mark duties/taxes paid on any booking.
|
||
**Fix:** add `@BookingStaff(FREIGHT_PERMS…)` guards.
|
||
|
||
### H11. `interchange-documents` controller has no authorization
|
||
**File:** `interchange-documents/interchange-documents.controller.ts` — `POST /generate-from-schedule`, `PATCH /:id/acknowledge|dispute|cancel` reachable by any authenticated user. These are outward-facing customs/port handover records.
|
||
**Fix:** staff permission guards.
|
||
|
||
### H12. Anyone can sign anyone's contract as CUSTOMER
|
||
**File:** `contracts/contracts.controller.ts:502-523` (asserts permission only for non-CUSTOMER roles) + `contract-transition.service.ts:795-816` (CUSTOMER branch checks only status + a **caller-supplied** `dto.otpPhone`/`dto.otp`). Any portal user signs another company's CONTRACT_READY contract with an OTP on their own phone → SIGNED_CUSTOMER, firing counter-sign/clearance-fee. `renew` (525) and `clearance/documents` (543) have the same gap.
|
||
**Fix:** call `assertCustomerCanAccessContract`; verify OTP against the contract company's registered phone, not `dto.otpPhone`.
|
||
|
||
### H13. Any file downloadable by anyone with the UUID *(live-confirmed public)*
|
||
**File:** `files/files.controller.ts:20-55` — the single global file-stream route is `@Public()`, no auth, no ownership, no expiry. All uploads funnel through it: driver documents, contract PDFs, company license / Fayda national-ID files.
|
||
**Repro (live):** `GET /api/files/{uuid}` → `404` with no token (reached handler; a valid id streams the file). Control routes return `401`.
|
||
**Fix:** require `JwtGuard` and authorize by the file's `resource`/`resourceId`, or serve via short-lived signed URLs (the code already has `filesService.signUrl`).
|
||
|
||
### H14. Duplicate `payment.succeeded` regresses advanced/cancelled bookings back to PAID
|
||
**File:** `bookings/booking-invoice.service.ts:133-166` — the idempotency guard `// if (booking.paymentStatus === "PAID") return;` is **commented out**, then it unconditionally rewrites `status: "PAID"` and re-runs allocation. The relay is documented at-least-once and freight never dedupes `eventId` (the `PaymentWebhookEventEntity` is registered but unused).
|
||
**Impact:** a replayed success on a `SCHEDULED`/`DISPATCHED` — or `CANCELLED` — booking force-rewrites it to PAID and re-fires side effects.
|
||
**Fix:** restore the guard as a state-machine check (only advance from awaiting-payment statuses); persist processed `eventId`s.
|
||
|
||
### H15. Booking prices ignore the contract's frozen rate snapshots
|
||
**File:** `bookings/booking-pricing.service.ts:123-229` prices exclusively from `ratesService.findLiveRates()`; nothing reads `contract_rate_snapshots` (only the clearance fee honors the freeze). `contract-booking.service.ts:289-295` comments "compute from contract unit rates" but calls the live-rate path.
|
||
**Impact:** customer signs a contract at 1,916 USD/40ft, rates team raises the live rate to 2,300, the drawdown booking bills 2,300 — contradicting the signed contract PDF.
|
||
**Fix:** in `computePriceForBooking`, when `booking.contractId` is set, resolve unit prices from that contract's snapshots (fall back to live only for un-frozen codes).
|
||
|
||
### H16. Cancelling a booking leaves its invoice open & payable; refund path is dead code
|
||
**File:** `booking-transition.service.ts:519-543` (`cancel()` writes a note + `status: CANCELLED`, never cancels the open PREPAID invoice) and `billing.service.ts:654-672` (`markInvoiceAsRefunded` has **zero callers**).
|
||
**Impact:** the cancelled booking's invoice stays payable in the portal; paying it fires `booking.invoice.paid` → flips the CANCELLED booking back to PAID (see H14). Money already collected on a later-cancelled flow has no refund mechanism.
|
||
**Fix:** `cancel()`/`reject()` must cancel/expire the open invoices in the same transaction; wire `markInvoiceAsRefunded` to a real staff refund endpoint; make `advanceBookingOnPayment` refuse terminal-status bookings.
|
||
|
||
### H17. `cancelTrainSchedule` has no status guard — a DISPATCHED/ARRIVED train can be cancelled
|
||
**File:** `train-scheduling/train-scheduling.service.ts:3226-3311` (unguarded at `train-scheduling.controller.ts:817`). Every sibling transition checks `status`; this one goes straight to the cancel transaction.
|
||
**Impact:** cancelling a DISPATCHED train sets every pinned wagon's `currentYardId = originStationId` ("never left") while they're rolling; releases the locomotives out on this run to AVAILABLE; detaches IN_TRANSIT bookings while their cargo stays IN_TRANSIT. The frontend only shows Cancel for `DRAFT`/`SCHEDULED` (`TrainScheduleV2ListPage.tsx:439-469`) — the API enforces nothing.
|
||
**Fix:** reject unless `['DRAFT','SCHEDULED'].includes(schedule.status)`.
|
||
|
||
### H18. `executeReschedule` is not transactional — a mid-flight failure strands the schedule half-rescheduled
|
||
**File:** `scheduling-reschedule/scheduling-reschedule.service.ts:131-210` — four separately-committed steps (persist new date → unassign each displaced booking → `assignBookingsToSchedule` → audit event). Step 3 routinely throws (re-validates against the *new* date persisted in step 1). Result: date already moved, displaced bookings already gone, no audit event, `400` to staff.
|
||
**Fix:** run the whole execute in one `dataSource.transaction`, threading the manager through unassign/assign.
|
||
|
||
---
|
||
|
||
## 5. MEDIUM — scheduling, bookings, cargo, containers
|
||
|
||
### M1. Cargo delivery never releases the container (counts itself)
|
||
**File:** `cargoes/cargoes.service.ts:159-175` — sets `cargo.status = 'DELIVERED'` in memory, then counts `LOADED` cargo on the container *before saving*, so the cargo being delivered counts itself → `remaining >= 1` always → the container is **never** flipped back to AVAILABLE.
|
||
**Fix:** save the cargo first, or exclude the current id: `count({ where: { containerId, status: 'LOADED', id: Not(cargo.id) } })`.
|
||
|
||
### M2. Cargo unload leaves the container marked LOADED
|
||
**File:** `cargoes.service.ts:139-147` — `unloadCargo` sets cargo `UNLOADED` but never touches `container.status` (which `loadCargo` set to LOADED). Emptied containers read as in-use forever.
|
||
**Fix:** on unload, if no remaining LOADED cargo references the container, reset it to AVAILABLE.
|
||
|
||
### M3. Container↔wagon assignment: no capacity/duplicate guard; status wrongly AVAILABLE
|
||
**File:** `container-management/containers.service.ts:113-136` — `MAX(position)+1` check-then-act with no `(wagon_id, position)` unique constraint; no check that the container is already on another wagon (silently overwrites `wagonId`); sets `status = 'AVAILABLE'` for a container physically on a wagon (so it reads free for another assignment). *(There's also a dead `containers.service copy.ts` duplicate.)*
|
||
**Fix:** reject when the container already has a `wagonId`; enforce wagon capacity; unique `(wagon_id, position)` + allocate in a transaction; use a distinct on-wagon status.
|
||
|
||
### M4. Interchange document state machine holes
|
||
**File:** `interchange-documents/interchange-documents.service.ts:182-206` — `dispute` has **no status guard** (a CANCELLED or ACKNOWLEDGED doc can be flipped to DISPUTED); `acknowledge` guards only CANCELLED, so an already-DISPUTED doc can be quietly ACKNOWLEDGED (losing the dispute).
|
||
**Fix:** restrict `dispute` to GENERATED/ACKNOWLEDGED; restrict `acknowledge` to GENERATED.
|
||
|
||
### M5. Interchange item weight mixes tons and kg in one column
|
||
**File:** `interchange-documents.service.ts:238-361` — the `weight` column is `COALESCE`d from tons sources (`total_vgm_tons`, `cargo_total_weight_vgm`) *and* kg sources (`containers.max_gross_weight`, `cargoes.weight // kg`). Different line items in the same customs handover carry weights ~1000× apart.
|
||
**Fix:** normalize every source to one unit before writing.
|
||
|
||
### M6. Cargo load/create allow weight over container capacity
|
||
**File:** `cargoes.service.ts:114-137` — `loadCargo` sets `cargo.weight = dto.weight` (`@Min(0)` only), never compared to the container's `maxGrossWeight`, no aggregate across cargoes.
|
||
**Fix:** verify `tare + sum(loaded) ≤ maxGrossWeight` (mind M5's units) and reject overflow.
|
||
|
||
### M7. Reschedule changes the departure date without any of `updateScheduleDate`'s validation
|
||
**File:** `scheduling-reschedule.service.ts:151-157` vs `train-scheduling.service.ts:781-865` — the reschedule/maintenance path writes `scheduledDepartureDate` directly: no `PRE_WINDOW` check, no lead-window rejection, no re-derivation of `windowOpensAt/ClosesAt/Phase`, no route+day group re-anchor, and **past dates are accepted** (never compared to `now`; only the dialog checks client-side). Windows keep the timing computed for the OLD date.
|
||
**Fix:** delegate to `updateScheduleDate` (or replicate its checks) and validate `newDepartureDate > now`.
|
||
|
||
### M8. `compareSchedulingPriority` sorts null-date bookings FIRST (comment says last)
|
||
**File:** `scheduling/compare-scheduling-priority.util.ts:4-6,20-22` — null `scheduledDate` → `getTime()` falls back to `0` (epoch), ascending sort puts it first. In `previewReschedule` this decides who is retained when capacity is tight → a general-contract booking with no date outranks customers who booked a concrete slot.
|
||
**Fix:** fall back to `Number.MAX_SAFE_INTEGER`, not `0`.
|
||
|
||
### M9. Assigning bookings never checks the booking's day matches the schedule's departure day
|
||
**File:** `train-scheduling.service.ts:1226-1259` + `3321-3618` — the parity guard's comment claims day-fit is enforced "downstream", but `validateBookingsForScheduling` never reads `dto.scheduleDate` or compares `booking.scheduledDate`. A booking a customer picked for Jul 25 can be assigned to a train departing Jul 17, silently.
|
||
**Fix:** add a violation (or `forceAssign` warning) when `eatDay(booking.scheduledDate) !== eatDay(schedule departure)`.
|
||
|
||
### M10. Partial `unassignBooking` frees the booking but not its wagon slots/totals
|
||
**File:** `train-scheduling.service.ts:1492-1541` — `TrainSetWagon` slots and `TrainSet` aggregates are only reset when the train becomes fully empty. Remove 1 of 3 bookings → tonnage/length/wagonCount stay stale, empty slots stay RESERVED with wagons pinned, free-capacity under-reports (can hide the day from customers), dispatch sends the empty pinned wagons.
|
||
**Fix:** after a partial unassign, release the emptied slots and recompute totals from surviving allocations.
|
||
|
||
### M11. Displaced-booking fallback leaves the booking still linked to the schedule
|
||
**File:** `scheduling-reschedule.service.ts:159-168` — on unassign failure the `catch` only flips scheduling fields; it doesn't delete the `TrainScheduleBooking` link, clear `trainScheduleId`, or free allocations. The booking becomes ELIGIBLE for batch fills **and** still linked → double-booking; its stale `trainScheduleId` also blocks manual assignment elsewhere.
|
||
**Fix:** in the fallback, delete the link + allocations and set `trainScheduleId: null` (or re-throw and abort).
|
||
|
||
### M12. Reschedule notifies "rescheduled to a new date" even when the date didn't change
|
||
**File:** `scheduling-reschedule.service.ts:204-235` — `effectiveDeparture` falls back to the (never-null) existing date, so the `if (newDeparture)` branch always runs → a GOVERNMENT_PREEMPT rebalance with no date change SMS/email-blasts every retained customer "rescheduled to `<same date>`".
|
||
**Fix:** `const effectiveDeparture = dto.newDepartureDate ? new Date(dto.newDepartureDate) : null;`
|
||
|
||
### M13. Maintenance reschedule endpoint bypasses DTO validation & drops caller bookings
|
||
**File:** `scheduling-reschedule.controller.ts:56-65` (body typed as an intersection `PreviewRescheduleDto & { … }` → Nest emits `Object` metadata → **ValidationPipe is skipped**, so `newDepartureDate: "garbage"` and missing arrays reach the service) + `scheduling-reschedule.service.ts:257-283` (preview uses `currentIds.length ? currentIds : dto.incomingBookingIds`, so caller-supplied incoming ids are dropped on a non-empty train, then execute re-runs with `dto.incomingBookingIds` → the displaced-set equality check can 400).
|
||
**Fix:** real `MaintenanceRescheduleDto extends PreviewRescheduleDto` with `@IsDateString() newDepartureDate`; merge `currentIds ∪ dto.incomingBookingIds` for both preview and execute.
|
||
|
||
### M14. Routes editable (milestones deleted, endpoints swapped) while live schedules reference them
|
||
**File:** `routes/routes.service.ts:108-140` — `update()` deletes+rewrites milestones and origin/destination with no check for DRAFT/SCHEDULED/DISPATCHED schedules on the route. Schedules read the corridor live afterward (sub-leg validation, checkpoints, customer day pool), so a reroute silently invalidates boarding bookings and renumbers stations mid-run.
|
||
**Fix:** reject milestone/endpoint changes when any non-terminal schedule references the route (allow status-only edits).
|
||
|
||
### M15. Workspace capacity meter sums locomotive limits; API caps at the weakest locomotive
|
||
**File:** `.../components/trainScheduling/ScheduleWorkspacePanel.tsx:98-109` (`reduce(sum + maxPullWeightTons)`) vs `train-capacity.util.ts:242-260` + service `1349-1389` (`minLocomotiveLimits` — the weakest locomotive caps the train; gate is cargo **+ consist tare**). Two 3500T locos → the meter shows 7000T and "43% full" while the API already rejects adds at ~3500T gross; the overfill warning fires on the wrong threshold.
|
||
**Fix:** capacity = `min(maxPullWeightTons) + min(overageToleranceTons)`, and include consist tare in `used`.
|
||
|
||
### M16. Vehicles under maintenance stay assignable; maintenance never changes availability
|
||
**File:** `maintenance/*` never writes `vehicle.status`/availability; `maintenance.service.ts:36-46` COMPLETED doesn't restore anything; assignment paths check only availability, never `VehicleStatus.MAINTENANCE`.
|
||
**Impact:** a vehicle in the shop can be dispatched; marking a vehicle MAINTENANCE doesn't block assignment.
|
||
**Fix:** on maintenance start set the vehicle unavailable, on completion restore, and reject MAINTENANCE/OUT_OF_SERVICE/RETIRED at assignment.
|
||
|
||
### M17. Fuel purchases accept negative/zero quantities, no duplicate guard
|
||
**File:** `fuel/dto/create-fuel-purchase.dto.ts:11-15` (`liters`, `costPerLiter` are `@IsNumber()` only) → `fuel.service.ts:18` `totalCost = liters * costPerLiter`. Negative liters → negative monthly totals & averages, poisoning Financial Reports/Fleet Dashboard; no `(vehicleId, receiptNumber)` uniqueness → double-counting.
|
||
**Fix:** `@IsPositive()` on both; reject duplicate `(vehicleId, receiptNumber)`.
|
||
|
||
### M18. Public OTP verify: no expiry, no rate limit, no attempt cap, replayable
|
||
**File:** `otp/otp.service.ts:94-120` (`verifyOtp`, exposed `@Public()` at `otp.controller.ts:22-63`) — a 6-digit code (1e6 space) with unlimited attempts, no age check, and only flagged `verified=true` on success (the same code keeps working). The hardened `verifyOtpForAction` (TTL + 5-attempt cap + delete-on-success) exists but this route doesn't use it.
|
||
**Fix:** give `verifyOtp` the same TTL/attempt-cap/consume semantics; rate-limit the public OTP routes.
|
||
|
||
### M19. OTP generated with `Math.random()` (not a CSPRNG)
|
||
**File:** `otp/otp.service.ts:27-29` — this gates password reset (`forgot-password.service.ts:134`) and contract-signature sudo. Predictable codes weaken account-takeover resistance.
|
||
**Fix:** `crypto.randomInt(100000, 1000000)`.
|
||
|
||
### M20. OTP send is public & unthrottled — SMS/email bombing + counter reset
|
||
**File:** `otp/otp.controller.ts:33-42` (`@Public() POST /otp/send`) + `otp.service.ts:35-88` (each send does `actionAttempts.delete(...)`, resetting the in-memory brute-force counter — attacker-controllable, and per-process anyway).
|
||
**Fix:** rate-limit per target + per IP; move the attempt counter to persistent storage.
|
||
|
||
### M21. Email notifications silently discarded while reporting success
|
||
**File:** `notifications/strategies/notification.email.strategy.ts:8-11` — `send()` logs and `return false;` (stub). `notifications.service.ts:25-32 directSend` awaits it, logs `is sent - false`, and neither throws nor surfaces the failure. Every email notification (booking lifecycle, contracts, companies, booking-window) silently never sends.
|
||
**Fix:** implement the strategy (or route through the working `EmailClientService`); make `directSend` treat `false`/throw as an observable failure.
|
||
|
||
### M22. Rate "CEO approval" is self-approvable (no segregation of duties)
|
||
**File:** `rule-engine/services/rates.service.ts:186-208` + `rates.controller.ts:61-76` — `submit` and `approve` share the identical `@RuleEngineManage('rates')` permission, and `approve` never checks `approverUserId !== proposedByStaffId`. One staffer can draft→submit→approve LIVE. (Same in `priority-rule-change-requests.service.ts:78-112`.)
|
||
**Fix:** distinct approver permission + reject self-approval.
|
||
|
||
### M23. Uploads accept arbitrary type & unbounded size
|
||
**File:** `drivers/drivers.controller.ts:74-82` (`AnyFilesInterceptor()`, no `limits`/`fileFilter`) + `files/files.service.ts:36-55` (stores whatever mime/size). `file.buffer` held in memory → DoS; executables/active-HTML then served inline via the public files route (H13). The `file-upload-settings` config exists but isn't enforced.
|
||
**Fix:** Multer `limits.fileSize` + mime allowlist (driven by file-upload-settings), validated in `FilesService.upload`.
|
||
|
||
### M24. Incidents controller: full CRUD + IDOR for any authenticated user
|
||
**File:** `incidents/incidents.controller.ts:17-69` — no permission guard on any route (sibling fleet modules all use `@BookingStaff`). A portal customer can read any driver's incident history by `driverId` and create/alter/delete incident records.
|
||
**Fix:** class-level `@BookingStaff(FREIGHT_PERMS.incidents.view)` + per-write permissions.
|
||
|
||
### M25. Tracking timeline IDOR
|
||
**File:** `tracking/tracking.controller.ts:11-17` — `GET /:consignmentId` has no guard/ownership check; any logged-in user reads any consignment's full movement history by iterating UUIDs.
|
||
**Fix:** permission guard + scope to the caller's company.
|
||
|
||
### M26. Clearance-fee gate silently waived when the fee snapshot is missing
|
||
**File:** `contracts/clearance-fee.service.ts:80-87` — `gateApplies` returns `false` (skipping the prepay gate, warn-log only) whenever no `CUSTOMS_CLEARANCE` snapshot line exists, and `contract-pricing.service.ts:262-284` falls back to a stale stored breakdown. A customs contract whose price predates the fee feature ships clearance for free.
|
||
**Fix:** for `customsClearingEnabled` contracts, hard-fail counter-sign/shipment-request when no fee line resolves.
|
||
|
||
### M27. Manual settlement / `updateStatus` gaps in the invoice state machine
|
||
**File:** `billing.service.ts:577-609` (`recordPayment` guards Cancelled/Refunded/Paid but not Draft/Expired → an expired invoice can be settled at the counter, resurrecting a released flow) and `889-907` (`updateStatus` accepts **any** target including Paid without touching `paidAmount`/`balanceAmount` → a PAID invoice with a full outstanding balance).
|
||
**Fix:** add Draft/Expired to the reject list; restrict `updateStatus` to the Draft→Issued transition it's actually used for.
|
||
|
||
---
|
||
|
||
## 6. LOW — hardening & latent bugs
|
||
|
||
- **L1. Decommission ignores coupling** — `locomotives.service.ts:121-133` sets OUT_OF_SERVICE with no `TrainLocomotive` check *(live-confirmed: decommissioned a coupled loco, 200)*. Train keeps hauling with a dead loco on paper; `capacityTons` not recomputed. → link-table guard.
|
||
- **L2. Builder accepts ASSIGNED/UNAVAILABLE locomotives** — `train-builder.service.ts:653` blocks only OUT_OF_SERVICE/MAINTENANCE; a loco out on a dispatched run (ASSIGNED) can be coupled to a new train. → allowlist `AVAILABLE/IMPORT_READY/EXPORT_READY`.
|
||
- **L3. Legacy `PATCH /trains/:id`** — `trains.service.ts:50-55` `Object.assign` lets `status`/`capacityTons`/`code`/`trainNumber` be rewritten (unfreeze a dispatched train, break the weakest-loco capacity, code/number clashes → 500 or silent collision). → strip those fields; run the builder's clash query.
|
||
- **L4. `PATCH /train-builder/:id/details` skips even/odd run-number rule** — `update-train-details.dto.ts:17-29` lacks the `@Matches(/…[13579]$/ | …[02468]$/)` the build DTO enforces → can set IMPORT number odd. → copy the decorators.
|
||
- **L5. Legacy reorder ignores `trainId`** — `wagons.service.ts:271-287` (`_trainId` unused) renumbers any wagon list to `1..n` with no set-equality check → duplicate sequences on other trains. → validate set equality vs `find({ where: { trainId } })`.
|
||
- **L6. Wagon type soft-deletable while in use** — `wagon-types.service.ts:114-117` no reference check; soft-deleted type → `wagon.wagonType` loads null → builder length/tare math silently zeroes. → block when wagons reference it.
|
||
- **L7. Facilities create/update are dead** — `facilities/dto/*.dto.ts` are plain interfaces (no decorators) under a `forbidNonWhitelisted` pipe → every non-empty body 400s, empty body 500s. → decorate the DTOs + code-uniqueness check.
|
||
- **L8. Yard soft-delete has no reference guard** — `rule-engine/services/yards.service.ts:62-66` strands trains/locos/wagons parked there (their `current_yard_id` scalar survives but joins miss). → count references, 409.
|
||
- **L9. Wagon create incoherent state** — `wagons.service.ts:27-37` accepts `trainId` while status defaults AVAILABLE (violates attached⇒ASSIGNED); duplicate `wagonNumber` → 500 not 409. → drop `trainId`/`sequenceNumber` from create DTO, uniqueness pre-check.
|
||
- **L10. Locomotive code generation is a read-scan race** — `locomotives.service.ts:40-48` scans all rows for max `LOCO-NNN`; concurrent creates collide → 500. → `MAX(SUBSTRING…)` + probe loop or a sequence.
|
||
- **L11. `Train.capacityTons` stale after a coupled loco's pull limit is edited** — `locomotives.service.ts:87-119` has no recompute hook; list vs detail disagree. → recompute for trains found via `TrainLocomotive`.
|
||
- **L12. `GET /facilities/:id` returns `200 null`, `DELETE` succeeds for unknown ids** — `facilities.service.ts:20-30` (every sibling 404s). → throw NotFound.
|
||
- **L13. Import customs flow has no ordering/terminal guards** — `import-operations.service.ts:102-210`: duties-paid before notify, risk changed after completion, no terminal lock. → state + `completedAt` guards.
|
||
- **L14. Consignment create doesn't validate the referenced booking** — `consignments.service.ts:14-17` persists the DTO directly (`bookingId` only `@IsUUID`). → existence check.
|
||
- **L15. Incident & maintenance status transitions unvalidated** — `incidents.service.ts:64-71`, `maintenance.service.ts:36-46` accept any status (REPORTED→CLOSED, SCHEDULED→COMPLETED). → allowed-transition checks.
|
||
- **L16. Maintenance cost fields accept negatives** — `maintenance/dto/create-maintenance.dto.ts` `estimatedCost/actualCost/costAmount` no `@Min(0)`. → add it.
|
||
- **L17. Procurement depreciation accepts inverted money** — `procurement/dto/procurement.dto.ts:89-99` (no `@Min(0)`) + `procurement.service.ts:120-131`: `salvageValue > cost` → negative monthly depreciation → `bookValue` grows unbounded. → `@Min(0)` + `salvage <= cost`.
|
||
- **L18. `notification-inbox` fan-out is sequential N+1** — `notification-inbox.service.ts:62-64` per-recipient create+countUnread+findOne. → batch/parallelize (it's fire-and-forget, so slow not broken).
|
||
- **L19. GPS history `limit` unguarded vs NaN/negative** — `gps-tracking.controller.ts:38` passes `parseInt` through; `?limit=abc`→NaN, `?limit=-5`→negative take. → clamp to a positive int.
|
||
- **L20. Hardcoded default JWT secret** — `@edr/api-common` `shared-auth.module.js` falls back to a public constant if `JWT_SECRET` is unset (mitigated by the session-row check). → ensure `JWT_SECRET` always set in the environment.
|
||
|
||
---
|
||
|
||
## 7. Backoffice / portal frontend mismatches (already folded into findings above)
|
||
|
||
- **Locomotive/Wagon fleet edit forms PATCH the whole form object** (`FleetResourcePage.tsx:301`, `resources.ts:168-169,273-279`) → the accidental trigger for H1/H2.
|
||
- **`AssignWagonDialog.tsx:22-24`** — `status === Available || !w.trainId` should be `&&` (H5).
|
||
- **`WagonYardWorkspaceModal.tsx`** — "Assigned→Available" flip picks an arbitrary slice including coupled wagons (H6).
|
||
- **`ScheduleWorkspacePanel.tsx:98-109`** — capacity meter sums loco limits instead of the weakest (M15).
|
||
- **`InvoiceDetailPage.tsx:129-131` / `PayClearanceFeeButton.tsx:121-125`** — pay button shows `totalAmount`, backend charges `balanceAmount`; the correct `amountDue` line is commented out → a 60%-paid invoice tells the customer they'll pay the full total. Also a leftover `console.log(paymentMethod)` at line 71.
|
||
- **Reflected XSS on the public checkout page** — `billing/payment.controller.ts:100-103,156-176` interpolates provider error `message`/`status`/`intentId` unescaped into `@Public()` HTML. → HTML-escape or return a generic message.
|
||
- **Portal tracking/consignments pages are still mock-backed** (`portal/src/pages/tracking/shipments.mock.ts`, `consignments.mock.ts`) — they show mock data, not live consignments.
|
||
|
||
---
|
||
|
||
## 8. What is working correctly (verified, not bugs)
|
||
|
||
So the report is balanced — these were checked and are sound:
|
||
|
||
- **Train-builder same-yard enforcement** at build and attach (rejected my cross-yard attempts with clear 400s).
|
||
- **Double-coupling a locomotive** → `409 already coupled to train …` (guard works).
|
||
- **`train-builder` yard change** correctly relocates the whole consist (locos + wagons follow).
|
||
- **Dispatch / arrive / finalize** status guards, dispatch train-number pooling with pessimistic locks, route direction derivation and segment ordering, EAT-timezone math in `batch-window.util.ts` (no string date comparisons).
|
||
- **Booking cancellation** can't leak reserved capacity (only pre-reservation statuses cancellable); approval-step sequencing; `allocateContainers` and interchange generation are properly transactional.
|
||
- **GT06 GPS codec** coordinate/CRC/UTC decoding correct.
|
||
- **notification-inbox WebSocket auth**, **signatures**, **backoffice role management** (blocks reserved roles), **forgot-password/customer-reset** (use the hardened OTP + single-use IAM tickets) — all scope correctly.
|
||
- **Clearance-fee migration 2260** itself is sound (nullable/defaulted, `IF NOT EXISTS`); the gate enforcement on document upload is correct on both contract and booking sides (the bugs are around it — M26, and C1–C3 letting the fee be "paid" for free).
|
||
|
||
---
|
||
|
||
## 9. Boot-log noise (environment, not code bugs)
|
||
|
||
- **SMS + Email over RabbitMQ fail:** `Handshake terminated by server: 403 (ACCESS-REFUSED) … Login was refused using authentication mechanism PLAIN` — broker credentials for the dev environment. (Compounds M21: email is doubly dead — stub strategy *and* no broker.)
|
||
- **Swagger duplicate-DTO warnings:** `UpdateProfileDto`, `RequestChangesDto`, `SignContractDto` each defined twice with different schemas ("will throw in the next major version"). Rename the duplicates.
|
||
- **`LegacyRouteConverter` warning** on `/api/*` — path-to-regexp v6 wants `/api/*path`.
|
||
|
||
---
|
||
|
||
## 10. Test-data side effects from this audit (please review)
|
||
|
||
I drove real writes against `edr_freight`. Net state:
|
||
|
||
1. **Created train `TR-00002`** (`exportTrainNumber 9901`, `importTrainNumber 9902`, name "QA-TEST-TRAIN") — then **deleted it** (cleaned up).
|
||
2. **Destroyed wagon `BW1-0009`** (original UUID `905cad62-1b12-46a0-9268-bacbf115e787`) via the hard-delete bug (H3). **I recreated it via the API** — new UUID `d9b578ed-e65a-48f6-9ae2-fb8239451460`, `AVAILABLE`, in KALITY, same wagon type. The wagon count is whole again; only the UUID changed (its old `wagon_movements` ledger was cascade-deleted and cannot be recovered).
|
||
3. **Locomotives briefly moved** during the yard-divergence repro (LOCO-004, LOCO-023, LOCO-025) — **all restored** to KALITY / their original yards. LOCO-004 was briefly set OUT_OF_SERVICE by the decommission test and **restored to AVAILABLE**.
|
||
|
||
No other records were mutated. (A raw-SQL restore of the original wagon UUID was intentionally **not** performed — the DB-write guard blocked it, and re-inserting via the API is the correct, app-logic-respecting cleanup.)
|
||
|
||
---
|
||
|
||
## 11. Prioritized fix roadmap
|
||
|
||
**Do first (security / money — a customer or attacker can exploit these today):**
|
||
1. C1 — remove the DEMO auto-settle in `billing.service.ts:973`.
|
||
2. C2 / H13 — auth-guard `mark-paid`, `payments/initiate`, `payments/checkout`, `payments/receipt`, and `files/:id`.
|
||
3. C3 + C4 — unify currency units and enforce amount verification at settlement.
|
||
4. C5 — enable Telebirr/D-Money webhook signature checks.
|
||
5. C6 / H10 / H11 / H12 / M24 / M25 — add ownership/permission guards to booking writes, import-operations, interchange-documents, contract-sign, incidents, tracking.
|
||
|
||
**Do next (data integrity — the consist family, one structural fix):**
|
||
6. **H1–H7 + L1–L2**: the `TrainLocomotive`/`wagon.trainId` coupling guard on every locomotive/wagon mutation (this is your reported bug and its whole family). Switch `DELETE` to soft-delete.
|
||
7. H8/H9 capacity & consolidation locks; H17 cancel-status guard; H18 transactional reschedule.
|
||
|
||
**Then (correctness):** M1–M27 (container release, weight units, scheduling day/priority/notification bugs, OTP hardening, self-approval).
|
||
|
||
**Finally (hardening):** the L-series + the frontend dirty-field / display fixes in §7.
|
||
|
||
---
|
||
|
||
*Report generated from a live run of `pnpm run dev:freight` plus a full read of the in-scope modules. Every "live-confirmed" item has a reproduced request/response above; every code finding cites `file:line`.*
|
||
|
||
---
|
||
|
||
# ADDENDUM — Fixes Applied (2026-07-16)
|
||
|
||
All **18 High + 27 Medium** findings were fixed. Critical (C1–C6) and Low (L-series) were **left untouched** per the request (except L1, which shares the H1 guard). Applied across ~35 backend files + 3 frontend files via 6 partitioned edit passes.
|
||
|
||
**Verification status**
|
||
- `turbo run type-check --filter=@edr/freight-api` → **clean (0 errors)**.
|
||
- Web apps: freight-portal clean; freight-backoffice fails **only** on the pre-existing `user-management/web-Management/**` errors (documented, none in edited files).
|
||
- `scheduling-reschedule.service.spec` → **5/5 pass** (updated for the H18 transaction + a future-dated fixture for the new M7 past-date guard).
|
||
- App **boots clean** on 3030; migration `2280000000000-WagonNumberPartialUnique` applied and recorded (partial unique index `UQ_wagons_wagon_number_active` verified live).
|
||
|
||
**Live-verified fixes (real requests against the running app)**
|
||
- **H1** — `PATCH /locomotives/:id` on a coupled loco → `409 "coupled to train TR-00002; move the train instead"` (yard) and `409 "detach it before changing its status"` (status). *This is your reported bug — now behaves exactly as requested.*
|
||
- **L1** — decommission coupled loco → `409`.
|
||
- **H2** — `PATCH /wagons/:id` yard on coupled wagon → `409`; sending `trainId` → `400 "property trainId should not exist"`.
|
||
- **H3** — `DELETE /wagons/:id` on coupled wagon → `409`, wagon **survives** (data-loss bug closed); delete now uses `softRemove`.
|
||
- **H4** — `DELETE /trains/:id` on a built train → frees wagon (AVAILABLE, unlinked) + both locos, **soft-deletes** the train (tombstone row, not stranded).
|
||
|
||
**What each pass changed (high level)**
|
||
- **Fleet (H1–H7, L1):** coupling guard (`train_locomotives` / `wagon.trainId`) on every legacy loco/wagon mutation; `assignToTrain` tightened to builder rules; `bulk-status`/`bulk-transfer`/`unassign` refuse coupled/pinned wagons; hard-delete → soft-delete (+ migration); frontend `AssignWagonDialog` `||`→`&&`, workspace modal excludes coupled wagons.
|
||
- **Scheduling (H17, M8–M10, M14, M15):** cancel status-guard; null-date sort fixed; booking-day match check; partial-unassign now releases slots + recomputes TrainSet totals; route edit blocked while live schedules reference it; capacity meter uses weakest loco.
|
||
- **Reschedule (H18, M7, M11–M13):** date+event wrapped in one transaction; `newDepartureDate > now` guard; real `MaintenanceRescheduleDto` (validation no longer skipped); merged incoming-booking set; retained-customer notice only when the date actually moves; displaced-fallback clears `trainScheduleId`.
|
||
- **Bookings/Billing/Contracts (H8, H9, H12, H14–H16, M26, M27):** export-capacity + consolidation now lock the row (`FOR UPDATE`) and re-check before reserving; payment idempotency guard restored (won't resurrect CANCELLED/advanced); cancel/reject expire open invoices; contract-sign requires ownership + verifies OTP against the company's registered phone; pricing honors frozen contract-rate snapshots; clearance-fee gate hard-fails instead of waiving; invoice state-machine gaps closed.
|
||
- **Cargo/Containers/Customs (M1–M6, H10, H11):** container release counts fixed (exclude-self); unload frees container; load rejects over-capacity; container→wagon assign rejects double-assign + wraps position in a txn; interchange state-machine guarded; interchange item weights normalized to tons; import-operations + interchange-documents controllers now permission-guarded.
|
||
- **Ops/Auth/Files (H13, M16–M25):** file download requires auth; incidents + tracking guarded; maintenance flips vehicle status/availability; fuel rejects non-positive + duplicate receipts; OTP uses CSPRNG + TTL/attempt-cap/consume-on-success, send no longer resets the brute-force counter; email strategy implemented + failures surfaced; rate/priority self-approval blocked; driver uploads size/type-limited.
|
||
|
||
**Residual / partial (flagged in code with TODOs — intentional, safe)**
|
||
- **H18** cross-service (un)assign calls still run their own transactions (can't thread the manager without editing the scheduling core); date+event are atomic.
|
||
- **M7** kept the raw date write + `>now` guard; booking-window fields are not re-derived (delegating to `updateScheduleDate` would wrongly require `PRE_WINDOW`).
|
||
- **M11** best-effort detach (clears `trainScheduleId`); the link-row/allocation delete lives in the scheduling core.
|
||
- **M13** format is enforced; `newDepartureDate` is validated-when-present but not strictly required (parent DTO marks it optional).
|
||
- **M15** meter capped at weakest loco; consist tare isn't available client-side.
|
||
- **M16** maintenance sets vehicle status; the assignment-side reject lives in the excluded first/last-mile modules (out of scope).
|
||
- **M20** counter-reset removed; per-IP/per-target throttling is a TODO (no Throttler in the codebase yet).
|
||
- **M22** self-approval blocked for normal staff; **super admins are exempt** (full backoffice authority — verified live: propose→submit→self-approve → LIVE). Both rates and priority-rule change requests. A distinct CEO/approver permission is still recommended (TODO).
|
||
- **M24** reused real `drivers.*` permission keys (no `incidents:*` key exists yet — TODO to add one).
|
||
- **M25** view-guarded; company-scoping the query is a TODO.
|
||
- **H13** download now authenticated; ownership-by-resource + signed-URL previews are the next step (inline previews that relied on anonymous access will 401 until the frontend uses `signUrl`).
|
||
- **H15** frozen rates cover base rail + surcharges + first/last-mile; a rare container-fallback line keeps the live rate.
|
||
- **M3** position race narrowed by a txn; a `(wagon_id, position)` unique index is the full fix (TODO).
|
||
|
||
**Not committed.** All changes are in the working tree only.
|