From fd9191eca80b75e3fa85a5c4226ebb71470c6fef Mon Sep 17 00:00:00 2001 From: "s.esmaeili" Date: Mon, 12 Oct 2026 10:29:13 +0330 Subject: [PATCH] feat(orders): log the client price figures the server discards (MON-08) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 2 of the owner's MON-08 ruling. An app order is priced by the server (steps 1, 3, 4 landed with the wage-formula work), but the client's own wagePercent, discountPercent and flatRialCharge were being thrown away in silence — so a stale app build, a promotion that expired between the product page and checkout, and a deliberately tampered request were indistinguishable and left no trace anywhere. They are now logged with both numbers, the same pattern already used for the body's customerId. The pricing itself is untouched: the server's figures are what the order is charged, and an operator ordering from the panel still keeps the numbers they typed. Also adds the comparison test the ruling asked for, which step 3 never actually had — the suite only proved a tampered request gets overridden. The new test places one honest order as a customer and the same order as an operator and asserts the totals are identical, which is the real claim: an ordinary customer's total does not move. Two more cover the logging, including that an honest order logs nothing — so the quiet case cannot pass merely because the method never ran. Closes STUB-05 in the docs as well: the reference's six hardcoded staff numbers have been behind PROFILE_ADMIN_MOBILES / ORDER_ADMIN_MOBILES for a while, but its status still read "pending". api: 286/286 tests, 26 suites, typecheck clean. Co-Authored-By: Claude Opus 5 --- .../src/modules/orders/orders.service.spec.ts | 43 +++++++++++++++++ apps/api/src/modules/orders/orders.service.ts | 38 ++++++++++++++- docs/parity/CHANGELOG.md | 47 +++++++++++++++++++ docs/parity/ISSUES.md | 14 +++++- docs/parity/PROGRESS.md | 22 +++++++-- 5 files changed, 157 insertions(+), 7 deletions(-) diff --git a/apps/api/src/modules/orders/orders.service.spec.ts b/apps/api/src/modules/orders/orders.service.spec.ts index d9fbe5d..4063c62 100644 --- a/apps/api/src/modules/orders/orders.service.spec.ts +++ b/apps/api/src/modules/orders/orders.service.spec.ts @@ -1,3 +1,4 @@ +import { Logger } from '@nestjs/common'; import { Prisma } from '@prisma/client'; import { OrdersService } from './orders.service'; @@ -122,6 +123,48 @@ describe('OrdersService.createOrder — server-side making charge', () => { expect(created[0].totalGoldWeightGrams.toString()).toBe('5'); expect(created[0].flatRialCharge.toString()).toBe('999'); }); + + /** + * MON-08 step 3 (owner ruling): «تست مقایسه‌ای که نشان دهد نتیجه برای کاربر + * عادی یکسان است». An unmodified app sends what the server derives + * (DECISIONS §"Report — MON-08"), so deriving server-side must not move the + * total for an ordinary customer. + */ + it('gives an honest app exactly the total it sent', async () => { + const honest = { items: [{ productVariantId: 'v1', quantity: 1, wagePercent: 10, discountPercent: 0, flatRialCharge: 0 }] } as never; + const a = build(); + await a.service.createOrder(honest, 'c1', { placedBy: 'customer' }); + const b = build(); + await b.service.createOrder(honest, 'c1', { placedBy: 'operator' }); + expect(a.created[0].totalGoldWeightGrams.toString()).toBe(b.created[0].totalGoldWeightGrams.toString()); + expect(a.created[0].flatRialCharge.toString()).toBe(b.created[0].flatRialCharge.toString()); + }); + + /** MON-08 step 2: a discarded client figure must leave a trace. */ + it('logs what the client sent when it differs from the server', async () => { + const warn = jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); + try { + await build().service.createOrder(tampered, 'c1', { placedBy: 'customer' }); + const lines = warn.mock.calls.map((c) => String(c[0])).join('\n'); + expect(lines).toContain('wagePercent sent=0 used=10'); + expect(lines).toContain('discountPercent sent=50 used=0'); + expect(lines).toContain('flatRialCharge sent=999 used=0'); + } finally { + warn.mockRestore(); + } + }); + + it('stays quiet when the app sent the right figures', async () => { + const warn = jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); + try { + const honest = { items: [{ productVariantId: 'v1', quantity: 1, wagePercent: 10 }] } as never; + await build().service.createOrder(honest, 'c1', { placedBy: 'customer' }); + const pricing = warn.mock.calls.filter((c) => String(c[0]).includes('Order pricing')); + expect(pricing).toHaveLength(0); + } finally { + warn.mockRestore(); + } + }); }); /** diff --git a/apps/api/src/modules/orders/orders.service.ts b/apps/api/src/modules/orders/orders.service.ts index 33e0665..8b1d196 100644 --- a/apps/api/src/modules/orders/orders.service.ts +++ b/apps/api/src/modules/orders/orders.service.ts @@ -4,7 +4,7 @@ import { PrismaService } from '../../prisma/prisma.service'; import { CatalogService } from '../catalog/catalog.service'; import { LedgerService } from '../accounting/ledger.service'; import { AccountingHooksService } from '../accounting-sync/accounting-hooks.service'; -import { CreateOrderDto } from './dto/create-order.dto'; +import { CreateOrderDto, CreateOrderItemInputDto } from './dto/create-order.dto'; import { WageService } from '../pricing/wage.service'; import { SmsService } from '../sms/sms.service'; @@ -88,6 +88,7 @@ export class OrdersService { if (quotes && !quote) throw new NotFoundException(`Variant ${variant.id} not found`); const wagePercent = new Prisma.Decimal(quote ? quote.wagePercentSnapshot : item.wagePercent); const discountPercent = new Prisma.Decimal(quote ? quote.discountPercentSnapshot : (item.discountPercent ?? 0)); + if (quote) this.warnOnPriceMismatch(variant.id, item, wagePercent, discountPercent); const netPercent = wagePercent.sub(discountPercent); const computedWeightGrams = rashWeight.mul(netPercent.div(100).add(1)); @@ -173,6 +174,41 @@ export class OrdersService { return placed; } + /** + * MON-08 step 2: the server's own figures are what the order is charged, but + * a client that sent something else is worth seeing. An ordinary app sends + * exactly what the server derives (DECISIONS §"Report — MON-08"), so a + * mismatch means a stale app version, a promotion that expired between the + * product page and checkout, or a tampered request. + * + * `flatRialCharge` is included because the app hardcodes 0 (cart.vue:179) and + * only an operator ever sets one — anything non-zero from a customer is a + * client that should not be sending it. + */ + private warnOnPriceMismatch( + variantId: string, + sent: CreateOrderItemInputDto, + wagePercent: Prisma.Decimal, + discountPercent: Prisma.Decimal, + ) { + const differs: string[] = []; + if (!wagePercent.eq(sent.wagePercent)) { + differs.push(`wagePercent sent=${sent.wagePercent} used=${wagePercent.toString()}`); + } + if (!discountPercent.eq(sent.discountPercent ?? 0)) { + differs.push(`discountPercent sent=${sent.discountPercent ?? 0} used=${discountPercent.toString()}`); + } + if ((sent.flatRialCharge ?? 0) !== 0) { + differs.push(`flatRialCharge sent=${sent.flatRialCharge} used=0`); + } + if (differs.length) { + this.logger.warn( + `Order pricing for variant ${variantId}: the server's figures are used, ` + + `the client's are ignored — ${differs.join('; ')}`, + ); + } + } + /** * The admins who are told a new order was placed. * diff --git a/docs/parity/CHANGELOG.md b/docs/parity/CHANGELOG.md index f8e1127..9f2a493 100644 --- a/docs/parity/CHANGELOG.md +++ b/docs/parity/CHANGELOG.md @@ -829,3 +829,50 @@ sales-expert 33/33 · name trigger 9/9 · typecheck clean on all four apps. table was written. _No data was deleted in this entry._ + +--- + +## 2026-10-12 — Phase 2 — item #1: MON-08 step 2 (the discarded client price is now logged) + +- **What:** `OrdersService.warnOnPriceMismatch` — when an app order is priced by + the server and the client had sent different figures, the difference is logged + (`wagePercent sent=… used=…`, same for `discountPercent`, plus any non-zero + `flatRialCharge`). The order is still charged the server's figures; nothing + about the pricing changed. +- **Why:** step 2 of the owner's MON-08 ruling. Steps 1, 3 and 4 had landed with + the wage-formula work, but the client's values were being discarded **in + silence** — so a stale app version, a promotion that expired between the + product page and checkout, and a tampered request were indistinguishable and + left no trace at all. +- **Reference:** `henza_server/app/Http/Controllers/Api/ApiMobileController.php:536-541` + (debt computed from the request, nothing re-read from the DB); + `henza_pwa/src/views/cart.vue:170-179` (the app computes all three). +- **Target:** `apps/api/src/modules/orders/orders.service.ts` (the call site in + the per-item map; the method beside `adminMobiles`). +- **Matrix row:** MON-08 in `ISSUES.md` §"Phase 2 — item #1". +- **Commit:** see below. + +**Also added — the comparison test the ruling asked for.** Step 3 required «تست +مقایسه‌ای که نشان دهد نتیجه برای کاربر عادی یکسان است», and the suite did not +have one: it only proved that a *tampered* request gets overridden. The new +`"gives an honest app exactly the total it sent"` places the same honest order +once as a customer (server-priced) and once as an operator (client-priced) and +asserts the two totals are identical — which is the actual claim, that an +ordinary customer sees no change. Two more cover the logging itself: the +tampered order logs all three fields, and an honest one logs nothing (so a green +"quiet" test cannot be green merely because the method never ran). + +`orders.service.spec.ts`: 13 → 16 tests. Full api suite **286/286, 26 suites**, +`tsc --noEmit` clean, and the run log inspected for swallowed errors. + +**STUB-05 closed** in the same pass: the status still read "Pending the SMS +approval" though `PROFILE_ADMIN_MOBILES` and `ORDER_ADMIN_MOBILES` had replaced +the reference's six hardcoded staff numbers. No literal number is in the +target's git history. + +**Deliberately unchanged:** the target reads `rashWeightGrams` from the variant +row rather than trusting the client, unlike the reference. Per the owner's +earlier ruling this deviation stays, and it is re-recorded in `ISSUES.md` so it +is not "fixed" later by someone restoring parity for its own sake. + +_No data was deleted in this entry._ diff --git a/docs/parity/ISSUES.md b/docs/parity/ISSUES.md index 38e46eb..2ff9950 100644 --- a/docs/parity/ISSUES.md +++ b/docs/parity/ISSUES.md @@ -110,7 +110,7 @@ Severity: `Critical` (money loss / security / data loss) · `High` · `Medium` | ID | Severity | Where | Description | Evidence | Status | |---|---|---|---|---|---| | SEC-12 | **Critical** | REF `henza_server/app/Models/Sms.php:73-77,101-105` | Live SMS-panel production credentials (username, plaintext password, sender line) are **committed to git** in the reference, duplicated across two methods. An older provider's credentials sit commented out beside them. Anyone with repo access can send SMS on the owner's account at their cost. | Read the file. | Not ported — the target will read them from env only. **The owner should rotate these credentials**; they must be assumed compromised. | -| STUB-05 | Medium | REF `ApiMobileController.php:154-158,601-605`; `Adm/OrderController.php:54-55`; `Customer/OrderController.php:141-145` | Admin notification recipients are **hardcoded mobile numbers** scattered across controllers (six distinct numbers). Staff changes require a code deploy, and an ex-employee keeps receiving order data. | Read the files. | Port as `SMS_ADMIN_RECIPIENTS` (comma-separated, default empty) rather than copying the literals. Pending the SMS approval. | +| STUB-05 | Medium | REF `ApiMobileController.php:154-158,601-605`; `Adm/OrderController.php:54-55`; `Customer/OrderController.php:141-145` | Admin notification recipients are **hardcoded mobile numbers** scattered across controllers (six distinct numbers). Staff changes require a code deploy, and an ex-employee keeps receiving order data. | Read the files. | ✅ **FIXED** (2026-10-12, commit `83e5d28`) — ported as two comma-separated env vars, `PROFILE_ADMIN_MOBILES` and `ORDER_ADMIN_MOBILES`, both defaulting to empty. No literal number is in the target's git history. | | DEAD-06 | Low | REF `henza_pwa/src/views/cart.vue:181-182` | The cart sends `wage_weight: 5.7856000000000005` and `weight: 6.303744000000001` as **literal constants**, each with a `// how to get?!` comment. The server ignores both: it recomputes weight from `rash_weight`, and the `wage_weight` use is commented out (`ApiMobileController.php:549-551`). | Read both files. | Dead payload fields — do not port. | ## Found while applying the D0 / percent migrations (2026-09-22) @@ -199,3 +199,15 @@ Severity: `Critical` (money loss / security / data loss) · `High` · `Medium` | ID | Severity | Where | Description | Evidence | Status | |---|---|---|---|---|---| | **FAV-01** | High (data invisible) | TGT both apps `stores/favoritesStore.ts`; no API route | Favourites were **memory-only** in both apps (lost on reload), and the API had no favourites route — so the 874 favourites imported from legacy `major_favorites` into `CustomerFavorite` were never shown. Legacy kept them on the server (`POST add-to-fav`, `POST get-favorites`, `routes/api.php:84-85`). | Store header said "local-only … until that backend support exists". | ✅ Fixed 2026-10-08: `GET customer/me/favorites`, `POST customer/me/favorites/toggle` (customer from the token); both apps load on sign-in, clear on sign-out, toggle optimistically and roll back on failure. Verified on the imported data: a customer with 116 legacy favourites sees all 116. | + +## Phase 2 — item #1, MON-08 close-out (2026-10-12) + +| ID | Severity | Where | Description | Evidence | Status | +|---|---|---|---|---|---| +| **MON-08** | **High (money)** | REF `ApiMobileController.php:536-541`, `henza_pwa/src/views/cart.vue:170-179` vs TGT `apps/api/src/modules/orders/orders.service.ts` | The reference computed a customer's gold debt straight from `percent_pay`, `discount` and `rash_weight` **as sent in the request**, with nothing re-read from the database — so a crafted order could set its own making charge, its own discount and its own weight, i.e. decide how much gold the customer owed. | DECISIONS §"Report — MON-08"; the PHP block is quoted there in full. | ✅ **FIXED.** All four steps of the owner's ruling are in place. (1) An app order is priced by `WageService.quote` — `variantPercent + customerPercent − promotions` through the approved formula (`pricing/wage-formula.ts`, legacy's by default), read from the DB. (2) The client's figures are ignored and a mismatch is now logged (`warnOnPriceMismatch`, added 2026-10-12) — the same pattern as the `customerId` warning. (3) The comparison test `"gives an honest app exactly the total it sent"` proves an ordinary customer's total is unchanged. (4) The formula, its ordering and its rounding are untouched (D4). An operator ordering from the panel still keeps the numbers they typed, as legacy's order builder did. 16 tests on this service. | + +**Accepted deviation, re-confirmed:** the reference also trusted the client's +`rash_weight`; the target reads the weight from the variant row. Per the owner's +ruling the target's behaviour stays — identical whenever the client sends the +true weight (which the reference app always does, `cart.vue:178`), different +only for a forged request, which is the point. diff --git a/docs/parity/PROGRESS.md b/docs/parity/PROGRESS.md index c25fa85..11a4024 100644 --- a/docs/parity/PROGRESS.md +++ b/docs/parity/PROGRESS.md @@ -19,11 +19,11 @@ The "Current state" table below is the parity project's and was last updated 202 | Field | Value | |---|---| -| **Phase** | **Phase 1 — admin parity — COMPLETE, at the gate** | +| **Phase** | **Phase 2 — apps parity — item #1 done** (Phase 1 closed and committed, `83e5d28`) | | **Branch** | `main` (the parity branches were merged; see git log 2026-10-08..10-12) | -| **Status** | All in-scope sections closed: §2 §4 §5 §6 §7 §8 §9 ✅ · §3 ⏸️ out of scope by D16. **283/283** jest tests, typecheck clean. | -| **Next step** | **GATE question asked 2026-10-12 — waiting on the owner** before Phase 2 (apps parity). | -| **Blocked on** | the gate answer. Two additive items await an owner ruling: **PERM-02** and the open rows below. | +| **Status** | Phase 1: all in-scope sections closed (§2 §4 §5 §6 §7 §8 §9 ✅ · §3 ⏸️ by D16). Phase 2: **item #1 MON-08 ✅**. **286/286** jest tests, typecheck clean. | +| **Next step** | Phase 2 item #2 — **the 5 screens missing from both apps (D3)**. `02-APPS-MATRIX.md` still to be created. | +| **Blocked on** | nothing. Two additive items await an owner ruling when convenient: **PERM-02** and the open rows below. | | **Last updated** | 2026-10-12 | ### Phase 1 close-out (2026-10-12) @@ -195,7 +195,19 @@ Consolidated into `00-INVENTORY.md`. Issues found: `ISSUES.md` (26 entries). 3. **D13** — the retail storefront is preserved but out of scope; see `DECISIONS.md` §D13 and `PROTECTED.md` §H. -## Phase 2 — item #1 (fixed by the MON-08 ruling) +## Phase 2 — item #1 (MON-08) — ✅ DONE 2026-10-12 + +All four steps of the ruling are in place; see `ISSUES.md` §"Phase 2 — item #1". +Steps 1, 3 and 4 had landed with the wage-formula work; **step 2 — logging the +client figures that get discarded — was added 2026-10-12**, along with the +comparison test the ruling asked for («تست مقایسه‌ای»), which step 3 had not +actually had: the suite only covered a tampered request, not an honest one. +`orders.service.spec.ts` is now 16 tests. + +An operator ordering from the panel still keeps the numbers they typed — only an +app order is re-priced by the server. + +The original requirement, for the record: Before anything else in Phase 2: the server must derive `wagePercent`, `discountPercent` and `flatRialCharge` from the DB and **ignore** the client's