diff --git a/apps/api/scripts/auth-smoke.sh b/apps/api/scripts/auth-smoke.sh index cbddb30..4fa4e9c 100644 --- a/apps/api/scripts/auth-smoke.sh +++ b/apps/api/scripts/auth-smoke.sh @@ -178,6 +178,66 @@ if [ -n "${CODE_LOG:-}" ] && [ -f "$CODE_LOG" ]; then sed -n 's/.*"mobile":"\([^"]*\)".*/\1/p') [ "$who" = "$CUST_PHONE" ] && ok "mobile = $who" \ || bad "mobile = $who (want $CUST_PHONE)" + + # ------------------------------------------------ SEC-02: orders --- + # POST /orders was public with customerId in the body, so anyone could + # place an order on anyone's account — and an order lands on that + # customer's gold debt and ledger. + echo "Orders require a customer token:" + c=$(curl -s -o /dev/null -w '%{http_code}' -X POST "$API/orders" \ + -H 'Content-Type: application/json' \ + -d '{"customerId":"x","items":[{"productVariantId":"x","quantity":1,"wagePercent":7}]}') + [ "$c" = 401 ] && ok "POST orders without a token -> 401" \ + || bad "POST orders without a token -> $c (want 401)" + + c=$(curl -s -o /dev/null -w '%{http_code}' "$API/orders/any-id") + [ "$c" = 401 ] && ok "GET orders/:id without a token -> 401" \ + || bad "GET orders/:id without a token -> $c (want 401)" + + # A variant to order, and a customer that is NOT the token's owner. + VARIANT=$(curl -s "$API/catalog/products" | + sed -n 's/.*"variants":\[{"id":"\([^"]*\)".*/\1/p' | head -1) + if [ -n "$VARIANT" ]; then + echo "The body's customerId must be ignored in favour of the token:" + ORD=$(curl -s -X POST "$API/orders" -H "Authorization: Bearer $CTOK" \ + -H 'Content-Type: application/json' \ + -d "{\"customerId\":\"some-other-customer\",\"items\":[{\"productVariantId\":\"$VARIANT\",\"quantity\":1,\"wagePercent\":7}]}") + OWNER=$(printf '%s' "$ORD" | sed -n 's/.*"customerId":"\([^"]*\)".*/\1/p') + MY_ID=$(curl -s "$API/customer/me" -H "Authorization: Bearer $CTOK" | + sed -n 's/^{"id":"\([^"]*\)".*/\1/p') + [ -n "$OWNER" ] && [ "$OWNER" = "$MY_ID" ] \ + && ok "order landed on the token's customer ($OWNER), not the body's" \ + || bad "order landed on '$OWNER', expected '$MY_ID'" + + # orderNumber is unique per order, so it identifies the row we just made + # without having to parse nested item ids out of the response. + ORDNUM=$(printf '%s' "$ORD" | sed -n 's/.*"orderNumber":"\([^"]*\)".*/\1/p') + MYORD=$(curl -s "$API/customer/me/orders" -H "Authorization: Bearer $CTOK" | + tr '{' '\n' | grep -F "\"orderNumber\":\"$ORDNUM\"" | + sed -n 's/^"id":"\([^"]*\)".*/\1/p' | head -1) + if [ -n "$MYORD" ]; then + c=$(curl -s -o /dev/null -w '%{http_code}' "$API/orders/$MYORD" -H "Authorization: Bearer $CTOK") + [ "$c" = 200 ] && ok "own order readable -> 200" || bad "own order -> $c (want 200)" + fi + + # Someone else's order and a nonexistent one must be indistinguishable, + # or the status code itself confirms which ids exist. + c=$(curl -s -o /dev/null -w '%{http_code}' "$API/orders/definitely-not-an-order" \ + -H "Authorization: Bearer $CTOK") + [ "$c" = 404 ] && ok "unknown order id -> 404" || bad "unknown order id -> $c (want 404)" + + echo "Admin order read is admin-only:" + c=$(curl -s -o /dev/null -w '%{http_code}' "$API/orders/admin/any" -H "Authorization: Bearer $CTOK") + [ "$c" = 401 ] && ok "customer token on orders/admin/:id -> 401" \ + || bad "customer token on orders/admin/:id -> $c (want 401)" + c=$(curl -s -o /dev/null -w '%{http_code}' "$API/orders/admin/any" -H "Authorization: Bearer $SUPER") + case "$c" in + 401|403) bad "admin token on orders/admin/:id -> $c (guard rejected an admin)" ;; + *) ok "admin token on orders/admin/:id -> $c (through the guard)" ;; + esac + else + printf ' \033[33mSKIP\033[0m no product variant available to order\n' + fi else bad "correct code -> $c (want 200)" fi diff --git a/apps/api/src/modules/admin-orders/admin-orders.service.ts b/apps/api/src/modules/admin-orders/admin-orders.service.ts index 3b43734..9c55524 100644 --- a/apps/api/src/modules/admin-orders/admin-orders.service.ts +++ b/apps/api/src/modules/admin-orders/admin-orders.service.ts @@ -320,14 +320,21 @@ export class AdminOrdersService { }), ); - const order = await this.orders.createOrder({ - customerId: payload.customer_id, - items, - // The admin builder serves the omde (major/*) flow, so orders it creates - // belong to the wholesale channel — legacy wrote them to the `orders` - // table, which MajorOrderController::index listed. - channel: OrderChannel.WHOLESALE, - }); + // The customer id is now a separate argument rather than a body field, so + // the customer-facing route can take it from the caller's token (SEC-02). + // This path is different: an operator legitimately places an order FOR a + // customer, so it passes the one the builder selected. The route itself is + // admin-guarded, which is what makes that safe. + const order = await this.orders.createOrder( + { + items, + // The admin builder serves the omde (major/*) flow, so orders it creates + // belong to the wholesale channel — legacy wrote them to the `orders` + // table, which MajorOrderController::index listed. + channel: OrderChannel.WHOLESALE, + }, + payload.customer_id, + ); // legacy `sts`: advance the new order to the chosen delivery status. const target = this.statusFromLegacyCode(payload.sts); diff --git a/apps/api/src/modules/customer-auth/customer-auth.service.ts b/apps/api/src/modules/customer-auth/customer-auth.service.ts index d007946..7b75c5c 100644 --- a/apps/api/src/modules/customer-auth/customer-auth.service.ts +++ b/apps/api/src/modules/customer-auth/customer-auth.service.ts @@ -213,21 +213,31 @@ export class CustomerAuthService { * (ApiMobileController.php:47). */ private async deliver(mobile: string, code: string) { - const body = + // The exact text the reference sent (ApiMobileController.php:47), kept for + // whenever a gateway is wired up. It is built here and handed to the + // gateway; it is deliberately NOT what gets persisted (see below). + const smsText = `<#> code is ${code}\n` + `رمز عبور یکبار مصرف شما جهت ورود به اپلیکشن هنزا گلد ${code} می باشد\n` + `aMo6iFyUQaY`; + void smsText; // TODO: hand to the SMS provider once one exists (STUB-02) if (process.env.NODE_ENV !== 'production') { this.logger.log(`[dev] OTP for ${mobile}: ${code}`); } - // Recorded the same way the admin panel's own SMS sends are. + // An audit row, NOT the message body. + // + // SmsMessage.body is listed and full-text searchable on the admin panel's + // SMS grid (admin-misc.service.ts:136 allows `body`, :135 searches it), so + // persisting the real text would show every live OTP to any operator + // holding the `sms` rid — and the whole point of hashing codeHash is that + // the code exists nowhere in readable form. The code is redacted here. await this.prisma.smsMessage.create({ data: { audience: MessageAudience.SINGLE_NUMBER, recipients: mobile, - body, + body: 'کد یکبار مصرف ورود (متن پیام ثبت نمی‌شود)', sentCount: 1, }, }); diff --git a/apps/api/src/modules/orders/dto/create-order.dto.ts b/apps/api/src/modules/orders/dto/create-order.dto.ts index 6c3d828..c029120 100644 --- a/apps/api/src/modules/orders/dto/create-order.dto.ts +++ b/apps/api/src/modules/orders/dto/create-order.dto.ts @@ -25,8 +25,20 @@ export class CreateOrderItemInputDto { } export class CreateOrderDto { + /** + * IGNORED BY THE SERVER. The customer is taken from the caller's token. + * + * It was required, and the route was public, so anyone could place an order + * on any customer's account — which lands on their gold debt and their + * ledger. Logged as SEC-02 in docs/parity/ISSUES.md. + * + * Kept as an optional field only so the two apps, which still send it, do not + * fail validation mid-flight. OrdersService logs a mismatch and always uses + * the token. Removed from the DTO and from both apps in Phase 2. + */ + @IsOptional() @IsString() - customerId!: string; + customerId?: string; @IsOptional() @IsString() diff --git a/apps/api/src/modules/orders/orders.controller.ts b/apps/api/src/modules/orders/orders.controller.ts index a710e69..5b1f542 100644 --- a/apps/api/src/modules/orders/orders.controller.ts +++ b/apps/api/src/modules/orders/orders.controller.ts @@ -2,40 +2,59 @@ import { Body, Controller, Get, Param, Patch, Post } from '@nestjs/common'; import { OrdersService } from './orders.service'; import { CreateOrderDto } from './dto/create-order.dto'; import { UpdateOrderStatusDto } from './dto/update-order-status.dto'; -import { Public, RequirePermission } from '../auth/guards'; +import { RequirePermission } from '../auth/guards'; +import { CurrentCustomer, CustomerAuth } from '../customer-auth/customer.guards'; +import type { AuthenticatedCustomer } from '../customer-auth/customer-jwt.strategy'; /** - * Customer-facing order routes. + * Order routes. * - * POST / and GET /:id stay `@Public()` for now: the target has no customer - * session yet, so the customer id travels in the request body. The reference - * derived it from the `api-customer` guard instead - * (henza_server ApiMobileController.php:533 reads `auth()->id()`), which is what - * stage 2 of the security hotfix restores. Tracked as SEC-02 / B1-B2 in - * docs/parity/ISSUES.md. - * - * PATCH /:id/status was public too, which was worse in kind: it let an - * anonymous caller move any order's status AND attribute the change to any - * admin via the DTO's actorAdminUserId. It is an admin operation in the - * reference, it had no caller in this repo (the admin panel posts to - * admin/orders/status instead), so it is now behind the order-status rid. + * All three were `@Public()` until the security hotfix. The reference's + * customer-facing order endpoints sat behind its `api-customer` guard and read + * the customer from the token (`auth()->id()`, + * henza_server ApiMobileController.php:533); the target had no customer session + * at the time, so the id travelled in the body and the server trusted it. + * That meant an anonymous caller could place an order on anyone's account — + * and an order lands on that customer's gold debt and ledger. Now that the + * customer token exists, the reference's arrangement is restored. */ @Controller('orders') export class OrdersController { constructor(private readonly orders: OrdersService) {} - @Public() + /** The customer comes from the token; the body's `customerId` is ignored. */ + @CustomerAuth() @Post() - create(@Body() dto: CreateOrderDto) { - return this.orders.createOrder(dto); + create(@Body() dto: CreateOrderDto, @CurrentCustomer() me: AuthenticatedCustomer) { + return this.orders.createOrder(dto, me.id); } - @Public() - @Get(':id') - get(@Param('id') id: string) { + /** + * Unscoped read for operators. + * + * Declared BEFORE the `:id` route below: Nest matches in declaration order, + * so a literal segment that could also be read as a parameter has to come + * first or it is never reached. + */ + @Get('admin/:id') + @RequirePermission('order.list') + getAsAdmin(@Param('id') id: string) { return this.orders.getById(id); } + /** + * A customer reading one of their own orders. Scoped by token; someone + * else's order is a 404 rather than a 403, so ids stay unenumerable. + * + * Mounted under /orders/:id (not /customer/me/orders/:id) so the existing + * client paths keep working; it is the scoping that matters, not the shape. + */ + @CustomerAuth() + @Get(':id') + get(@Param('id') id: string, @CurrentCustomer() me: AuthenticatedCustomer) { + return this.orders.getOwnedById(id, me.id); + } + @Patch(':id/status') @RequirePermission('order.list.sts.submit') updateStatus(@Param('id') id: string, @Body() dto: UpdateOrderStatusDto) { diff --git a/apps/api/src/modules/orders/orders.service.ts b/apps/api/src/modules/orders/orders.service.ts index af274e8..19a7289 100644 --- a/apps/api/src/modules/orders/orders.service.ts +++ b/apps/api/src/modules/orders/orders.service.ts @@ -1,4 +1,4 @@ -import { Injectable, NotFoundException } from '@nestjs/common'; +import { Injectable, Logger, NotFoundException } from '@nestjs/common'; import { OrderChannel, OrderStatus, Prisma } from '@prisma/client'; import { PrismaService } from '../../prisma/prisma.service'; import { CatalogService } from '../catalog/catalog.service'; @@ -17,6 +17,8 @@ interface ComputedItem { @Injectable() export class OrdersService { + private readonly logger = new Logger(OrdersService.name); + constructor( private readonly prisma: PrismaService, private readonly catalog: CatalogService, @@ -29,9 +31,24 @@ export class OrdersService { * mirrors legacy ApiMobileController::setOrder. Debt is NOT posted here — * only once the order reaches DELIVERED_TO_AGENT (see updateStatus). */ - async createOrder(dto: CreateOrderDto) { - const customer = await this.prisma.customer.findUnique({ where: { id: dto.customerId } }); - if (!customer) throw new NotFoundException(`Customer ${dto.customerId} not found`); + /** + * @param customerId taken from the caller's token by the controller, NEVER + * from the request body. The body's `customerId` is ignored; see the note + * on CreateOrderDto. + */ + async createOrder(dto: CreateOrderDto, customerId: string) { + const customer = await this.prisma.customer.findUnique({ where: { id: customerId } }); + if (!customer) throw new NotFoundException(`Customer ${customerId} not found`); + + // The apps still send customerId in the body (removed from them in Phase 2). + // It is not used for anything; a mismatch means either a stale client or an + // attempt to order on someone else's account, and both are worth seeing. + if (dto.customerId && dto.customerId !== customerId) { + this.logger.warn( + `Order body carried customerId=${dto.customerId} but the token is ${customerId}; ` + + `the token wins.`, + ); + } const computedItems: ComputedItem[] = await Promise.all( dto.items.map(async (item) => { @@ -78,7 +95,7 @@ export class OrdersService { const order = await tx.order.create({ data: { orderNumber: this.generateOrderNumber(), - customerId: dto.customerId, + customerId, branchId: dto.branchId, channel: dto.channel ?? OrderChannel.RETAIL, status: initialStatus, @@ -138,6 +155,7 @@ export class OrdersService { }); } + /** Unscoped read. Only reachable from the admin-guarded route. */ getById(orderId: string) { return this.prisma.order.findUnique({ where: { id: orderId }, @@ -145,6 +163,22 @@ export class OrdersService { }); } + /** + * A customer reading one of their own orders. + * + * Someone else's order is reported as 404, not 403: a 403 would confirm the + * id exists, which is the same enumeration leak the id-addressed customer + * routes had. + */ + async getOwnedById(orderId: string, customerId: string) { + const order = await this.prisma.order.findFirst({ + where: { id: orderId, customerId }, + include: { items: true, statusEvents: true, ledgerEntries: true }, + }); + if (!order) throw new NotFoundException('سفارش یافت نشد'); + return order; + } + private generateOrderNumber(): string { const random = Math.random().toString(36).slice(2, 8).toUpperCase(); return `ORD-${Date.now()}-${random}`; diff --git a/apps/user-app-v2/src/views/CartView.vue b/apps/user-app-v2/src/views/CartView.vue index 042abac..cc4eca8 100644 --- a/apps/user-app-v2/src/views/CartView.vue +++ b/apps/user-app-v2/src/views/CartView.vue @@ -211,14 +211,18 @@ const goOrders = async () => { const submitOrder = async () => { cartData.notify = false - if (!session.customerId) { + // Gate on the token, not on a remembered id: the id alone is no longer a + // credential, and POST /orders now requires a customer JWT (SEC-02). + if (!session.isLoggedIn) { loginNotify.value = true return } try { cartData.loadingPage = true const order = await api.createOrder({ - customerId: session.customerId, + // Ignored by the server, which takes the customer from the token; still + // sent so older builds keep validating. Removed in Phase 2. + customerId: session.customerId ?? undefined, items: cart.items.map((item) => { const variant = cart.variant(item) return { diff --git a/apps/user-app/src/views/CartView.vue b/apps/user-app/src/views/CartView.vue index b526022..3daed90 100644 --- a/apps/user-app/src/views/CartView.vue +++ b/apps/user-app/src/views/CartView.vue @@ -189,14 +189,18 @@ const goOrders = async () => { const submitOrder = async () => { cartData.notify = false - if (!session.customerId) { + // Gate on the token, not on a remembered id: the id alone is no longer a + // credential, and POST /orders now requires a customer JWT (SEC-02). + if (!session.isLoggedIn) { loginNotify.value = true return } try { cartData.loadingPage = true const order = await api.createOrder({ - customerId: session.customerId, + // Ignored by the server, which takes the customer from the token; still + // sent so older builds keep validating. Removed in Phase 2. + customerId: session.customerId ?? undefined, items: cart.items.map((item) => { const variant = cart.variant(item) return { diff --git a/docs/parity/CHANGELOG.md b/docs/parity/CHANGELOG.md index 674e7d7..a46fb8f 100644 --- a/docs/parity/CHANGELOG.md +++ b/docs/parity/CHANGELOG.md @@ -176,3 +176,62 @@ surface entirely. `pnpm test` works for the first time: **7 passed** covering `faToEn`. `faToEn` moved to its own `digits.ts` so it can be tested without booting Nest. - Typecheck clean on api, user-app and user-app-v2; both apps build. + +--- + +## 2026-09-22 — Security hotfix, stage 3 — SEC-02 (orders) + OTP audit + +Branch `parity/security-hotfix`. The user declined to defer SEC-02 to Phase 2: +a public `POST /orders` taking `customerId` from the body let anyone place an +order on any customer's account, and an order lands on that customer's **gold +debt and ledger**. That is a money hole, so it is closed here. + +### 1. `POST /orders` is customer-authenticated + +- **What:** the route is behind `@CustomerAuth()`. `OrdersService.createOrder` + now takes `customerId` as a **separate argument**, supplied by the controller + from the token. The body's `customerId` is ignored entirely; a mismatch is + logged (`OrdersService` warn) and the token always wins. +- `CreateOrderDto.customerId` and the shared `CreateOrderDto` are **optional**, + so both apps keep validating while they still send it. Removed in Phase 2. +- **Reference:** `ApiMobileController::setOrder` read `auth()->id()` + (henza_server:533) behind the `api-customer` guard — this restores that. +- **Admin path unaffected:** `AdminOrdersService` places orders *for* a selected + customer and passes that id explicitly; its route is admin-guarded. + +### 2. `GET /orders/:id` is scoped to the owner + +- `getOwnedById(orderId, customerId)` matches on both, and a miss is **404, not + 403** — a 403 would confirm the id exists, the same enumeration leak the old + `/customers/:id` routes had. A nonexistent id and someone else's id are + indistinguishable. +- Operators use the new `GET /orders/admin/:id` behind rid `order.list`. + Declared **before** `:id` because Nest matches in declaration order. + +### 3. Both apps + +- `CartView.vue` in **both** apps now gates on `session.isLoggedIn` (the token) + rather than on a remembered `customerId`, which is no longer a credential. + +### 4. OTP audit (user request 3) + +- **No master code, no bypass, no hardcoded code** anywhere in the API or either + app — verified by grep for `master.?code|bypass|backdoor|1111|0000|1234|skipOtp|DEV_OTP|TEST_OTP`. + Exactly one code path issues a token, and only after `bcrypt.compare`. +- **SMS provider: none.** No gateway is wired up (STUB-02). In development the + code is logged once via `Logger`, guarded by `NODE_ENV !== 'production'`. +- **Fixed while auditing:** `deliver()` persisted the real message text — with + the code in it — into `SmsMessage.body`, which the admin SMS grid both lists + and full-text searches (`admin-misc.service.ts:135-136`). Any operator with + the `sms` rid could read live OTP codes, defeating the point of hashing + `codeHash`. The persisted row is now a redacted audit line; the real text is + built for the future gateway and not stored. The 9 test rows created during + this session were deleted. + +### Tests + +`apps/api/scripts/auth-smoke.sh` extended again — **53 passed, 0 failed**: +order routes 401 without a token, a spoofed body `customerId` ignored in favour +of the token, own order readable, unknown/foreign order id 404, and the admin +order route rejecting a customer token while admitting an admin one. +Typecheck clean on api, user-app, user-app-v2 and shared-types. diff --git a/docs/parity/ISSUES.md b/docs/parity/ISSUES.md index 81c7854..dd1166e 100644 --- a/docs/parity/ISSUES.md +++ b/docs/parity/ISSUES.md @@ -16,7 +16,7 @@ Severity: `Critical` (money loss / security / data loss) · `High` · `Medium` | ID | Severity | Where | Description | Evidence | Suggested fix | |---|---|---|---|---|---| | SEC-01 | Critical | REF `henza_server/routes/admin.php:12,272` | The whole admin route file is **unauthenticated** — the `Route::middleware('auth:api')->group(...)` wrapper is commented out at both ends, leaving all ~215 admin routes public (incl. `wallet/charge`, `users`, `roles`). | Verified by reading both lines. | n/a in reference (read-only). Drives TGT decision below. | -| SEC-02 | Critical | TGT `apps/api/src/modules/accounting/accounting.controller.ts:7`, `orders/orders.controller.ts:7`, `customers/customers.controller.ts:5`, `catalog/catalog.controller.ts:5`, `pricing/pricing.controller.ts:5` | 15 routes carry `@Public()`, including **ledger writes** (`POST accounting/repayments/*`, `entries/:id/confirm`) and `POST /api/orders` + `PATCH /api/orders/:id/status`. The public status DTO accepts a client-supplied `actorAdminUserId`, so a caller can attribute an action to any admin. | `grep -rn "@Public()"`; source comments say "pre-existing unauthenticated endpoint". | Introduce customer-session auth for the app-facing routes; move ledger/status writes behind the admin guard. **[replicated]** from SEC-01. | +| SEC-02 | ~~Critical~~ **FIXED — hotfix stage 3** | TGT `apps/api/src/modules/accounting/accounting.controller.ts:7`, `orders/orders.controller.ts:7`, `customers/customers.controller.ts:5`, `catalog/catalog.controller.ts:5`, `pricing/pricing.controller.ts:5` | 15 routes carry `@Public()`, including **ledger writes** (`POST accounting/repayments/*`, `entries/:id/confirm`) and `POST /api/orders` + `PATCH /api/orders/:id/status`. The public status DTO accepts a client-supplied `actorAdminUserId`, so a caller can attribute an action to any admin. | `grep -rn "@Public()"`; source comments say "pre-existing unauthenticated endpoint". | ✅ All closed across stages 1-3. Ledger + order-status behind admin rids; customer routes behind the customer JWT; `POST /orders` takes the customer from the token and ignores the body. Only `catalog/products`, `pricing/latest`, `home/slider` and `uploads/:filename` remain public, all by design. | | SEC-03 | ~~Critical~~ **FIXED — hotfix stage 2** | TGT `apps/user-app/src/views/user/LoginView.vue`, `apps/user-app-v2/` same | OTP is **not verified** — any 4-digit code logs the user in. This is a **parity gap**: the reference does verify it (`ApiMobileController.php:51-57`, mismatch → HTTP 422). | `DECISIONS.md` D6 | ✅ Real server-side OTP: bcrypt-hashed code, 2-min expiry, 5 attempts, single use, send throttle, no enumeration. Verified end-to-end by `auth-smoke.sh`. | | SEC-08 | ~~Critical~~ **FIXED — hotfix stage 2** | TGT `apps/api/src/modules/customers/customers.service.ts:6-23,44-50` + `customers.controller.ts:10` (`@Public()`) | **Full customer book exposed to anonymous callers.** Login downloads `GET /customers` and matches the phone client-side, so the endpoint returns **every** active customer with `fullName`, `mobile`, **`nationalCode`**, `goldDebtGrams`, `fiatDebtRial`, `goldCreditLimitGrams`, and the URLs of **national-ID / business-licence / shop scans** (and `GET uploads/:filename` is public too, so those URLs resolve). `passwordHash` is correctly excluded. Not possible in the reference, where `userVerfiy` returned **one** customer after the code matched. | Read `customers.service.ts:5-23`; `DECISIONS.md` D8 | ✅ Closed. The whole controller is deleted — no route accepts a customer id any more. Reads moved to token-derived `/customer/me*`. Stage 1 had only narrowed the fields, which left the ids and the `:id` routes exposed. | | SEC-09 | ~~Critical~~ **FIXED — hotfix stage 1** | TGT `apps/api/src/modules/accounting/accounting.controller.ts:7` | `@Public()` on the whole accounting controller exposes **5 routes, 4 of them writes**: reconcile a balance, record a gold repayment, record a rial repayment, confirm a ledger entry. Faithful to the reference (whose admin routes were all unauthenticated) but contradicts the confirmed decision D1. | `DECISIONS.md` §"@Public audit" group A | ✅ Done: rids `wallet.list`, `wallet.charge`, `document.create` ×2, `document.list.confirm`. Verified 401/403/pass-through by `apps/api/scripts/auth-smoke.sh`. | @@ -90,3 +90,10 @@ Severity: `Critical` (money loss / security / data loss) · `High` · `Medium` | STUB-02 | TGT `apps/api` | No OTP, no SMS provider, no payment gateway/callbacks, no cron, no websockets. SMS and notifications are DB-persistence only. | | STUB-03 | TGT both user apps | All list filtering and "pagination" is **client-side** over one flat `GET /catalog/products`; no server `page`/`limit` params. "Pagination" is a 20-item IntersectionObserver window. | | STUB-04 | TGT both user apps | Live price is **pull-on-mount only** (`GET /pricing/latest?karatPerMille=750`) — no socket, no SSE, no polling interval. `quotedAt` renders as a Jalali **date only**, so a stale rate looks current. | + +## Found during the stage-3 OTP audit + +| ID | Severity | Where | Description | Evidence | Status | +|---|---|---|---|---|---| +| SEC-11 | High | TGT `apps/api/src/modules/customer-auth/customer-auth.service.ts` (deliver) | The OTP message text, **code included**, was persisted to `SmsMessage.body`, which the admin SMS grid lists and full-text searches (`admin-misc.service.ts:135-136`). Any operator holding the `sms` rid could read live codes — defeating the bcrypt hashing of `codeHash`. | Read both files; grid `allowed` includes `body`. | ✅ Fixed in stage 3: the persisted row is a redacted audit line. | +| MON-08 | Medium | TGT `apps/api/src/modules/orders/orders.service.ts:40-52` | `wagePercent`, `discountPercent` and `flatRialCharge` are taken from the **client** on order creation, so a caller can set their own making charge. `rashWeightGrams` is correctly read from the variant. **The reference is worse** — `ApiMobileController.php:538-539,619` takes `rash_weight` from the client too — so this is a partial improvement already. Reported per the user's instruction to flag client-supplied pricing. | REF `:536-545,615-632` vs TGT `:40-52` | Log only (D4). Fix in Phase 2 alongside the price-formula work. | diff --git a/docs/parity/MIGRATION-STATE.md b/docs/parity/MIGRATION-STATE.md new file mode 100644 index 0000000..13db672 --- /dev/null +++ b/docs/parity/MIGRATION-STATE.md @@ -0,0 +1,127 @@ +# Migration state — investigated before writing D0 / percent + +Requested by the user before any new migration is written. All findings are from +**read-only** queries; nothing was reset and `_prisma_migrations` was not touched. + +## The five questions + +### 1. Is `20260917120000_customer_name_parts_and_percent` recorded as applied? + +**Yes — but as a no-op.** + +``` +migration_name 20260917120000_customer_name_parts_and_percent +finished_at 2026-09-18T16:03:33.956Z +rolled_back_at null +applied_steps_count 0 <-- zero steps ran +``` + +`finished_at` is set and `rolled_back_at` is null, so **Prisma considers it +applied and will never re-run it**. `applied_steps_count = 0` says no SQL +statement was ever executed by it. + +### 2. What is in its SQL file? + +**Comments only — no executable statement.** The file is a placeholder that +explains itself: it says the columns were added "directly against the live DB" +and that it was recreated as an empty no-op so `prisma migrate status` would +stop erroring on a missing directory. + +### 3. Does `schema.prisma` declare `firstName` / `lastName` / `productPercent`? + +**No.** `grep` over `prisma/schema.prisma` finds none of the three. The +`Customer` model has `fullName` and nothing else name-related. + +### 4. Do the columns exist in the database? + +**No.** Querying `information_schema.columns` for `Customer` and those names +returns only `fullName`: + +``` +firstName absent +lastName absent +productPercent absent +makingChargeOffsetPercent absent +fullName present +``` + +**So the placeholder file's own comment is wrong.** It claims the columns "already +exist live"; they do not, at least not in this database (`gold_banking` on +localhost). Whatever database that comment described, it was not this one. + +**Consequence for the runtime:** none, and this is the good news. Prisma Client +generates from `schema.prisma`, and the schema does not mention these fields +either. Schema and database agree — both lack the columns. There is **no drift +that could make Prisma Client throw at runtime**. The only thing that is wrong is +the placeholder's comment. + +### 5. `prisma migrate status` (read-only) + +``` +9 migrations found in prisma/migrations +Following migration have not yet been applied: +20260922000000_customer_otp +``` + +Note on that line: the OTP table **does exist** — stage 2 created it with +`prisma db execute`, which runs SQL without writing a ledger row. So the schema +is correct but the ledger entry is missing, and `migrate deploy` would try to +run it again. The SQL is written with `CREATE TABLE IF NOT EXISTS`, the indexes +with `IF NOT EXISTS`, and the FK inside a `DO $$ ... EXCEPTION WHEN +duplicate_object` block, so a re-run is harmless. Still worth resolving cleanly — +see the recommendation. + +### 6. Where did the file come from? + +``` +b80fa9b Redesign product detail page and fix icon/cart reactivity bugs +``` + +A single commit, and the message has nothing to do with migrations — the +placeholder was swept in as collateral to an unrelated frontend change. + +--- + +## Recommendation + +**The placeholder is recorded as applied, so per the user's rule it must not be +edited.** It stays exactly as it is. + +1. **Leave `20260917120000_...` untouched.** Editing a migration Prisma has + recorded would make its checksum mismatch and break `migrate status` for + everyone. Its only sin is a misleading comment, and a follow-up migration + makes that harmless. +2. **Write the D0 and `percent` work as a new, additive migration** with a fresh + timestamp. It adds the columns the placeholder never added. Written to be + idempotent (`IF NOT EXISTS`) so it is safe on a database where someone *did* + apply the columns by hand. +3. **Resolve the OTP ledger entry** with + `npx prisma migrate resolve --applied 20260922000000_customer_otp`. + That writes the ledger row without running SQL, which is exactly right here + since the table already exists. **Needs the user's approval before running**, + as it writes to `_prisma_migrations`. +4. **Never** `migrate reset`, and never edit `_prisma_migrations` by hand. + +--- + +## Naming: one field, not two + +The user asked for a single final name. **`productPercent` is dropped; +`makingChargeOffsetPercent` is the name.** + +Reason: the reference has **three unrelated things** all called `percent`, and a +vague name is how they got confused in the first place (the target currently +reads the wrong one — CUST-02): + +| Reference `percent` | Where | What it is | +|---|---|---| +| `customers.percent` | `customer_add.php:32` | a **per-customer making-charge offset**, used only to shift catalogue filter bounds (`ApiMobileController.php:391-396,76-77`) | +| `ConfigStatic['percent']` | `AdminController.php:903,994` | the **wallet cashback rate**: `total_weight × percent/100` | +| `order_item.percent` | `MajorOrderController.php:220` | the **per-line making charge** on a wholesale order | + +`productPercent` would read as "a percent belonging to a product", which is the +third meaning, not the first. `makingChargeOffsetPercent` says which of the +three it is. It is mapped back to the legacy key `percent` in `toRow()`, so the +admin panel's field name does not change. + +`productPercent` is not created, so there is no duplicate field. diff --git a/packages/shared-types/src/index.ts b/packages/shared-types/src/index.ts index 1823f07..e3662c0 100644 --- a/packages/shared-types/src/index.ts +++ b/packages/shared-types/src/index.ts @@ -64,7 +64,15 @@ export interface CreateOrderItemDto { } export interface CreateOrderDto { - customerId: string; + /** + * Ignored by the server, which takes the customer from the caller's token. + * + * It used to be required, on a public route, so anyone could place an order + * on any customer's account — landing on their gold debt and their ledger + * (SEC-02 in docs/parity/ISSUES.md). Optional here only so the two apps, + * which still send it, keep type-checking; both drop it in Phase 2. + */ + customerId?: string; branchId?: string; items: CreateOrderItemDto[]; }