parity(api,apps): close SEC-02 - orders take the customer from the token
The user declined to defer this to Phase 2, correctly: a public POST /orders with customerId in the body let anyone place an order on any customer's account, and an order lands on that customer's gold debt and ledger. POST /orders is now behind CustomerAuth. OrdersService.createOrder takes the customer id as a separate argument that the controller fills from the token; the body's customerId is ignored and a mismatch is logged. The field stays optional in the DTO and in shared-types only so both apps keep validating while they still send it -- they drop it in Phase 2. The admin builder is unaffected: it legitimately places orders for a chosen customer and passes that id explicitly, behind the admin guard. GET /orders/:id is scoped to the owner. A foreign or nonexistent id both return 404, never 403, so ids stay unenumerable. Operators read through the new GET /orders/admin/:id behind rid order.list, declared before the :id route because Nest matches in declaration order. Both apps' CartView now gates on session.isLoggedIn rather than a remembered customerId, which is no longer a credential. OTP audit (requested): no master code, no bypass, no hardcoded code anywhere; one code path issues a token and only after bcrypt.compare. No SMS provider is wired up -- in development the code is logged once, guarded by NODE_ENV !== 'production'. Fixed while auditing (SEC-11): deliver() persisted the real message text, code included, into SmsMessage.body, which the admin SMS grid lists and full-text searches. Any operator with the sms rid could read live codes, defeating the hashing of codeHash. The stored row is now a redacted audit line. Also logged MON-08: wagePercent/discountPercent/flatRialCharge are still taken from the client on order creation. The reference is worse -- it takes rash_weight from the client too -- so this is already a partial improvement; logged rather than changed, per D4. Tests: auth-smoke.sh now 53 passed, 0 failed, including a spoofed body customerId being ignored in favour of the token. Typecheck clean on api, user-app, user-app-v2 and shared-types. [ref: henza_server ApiMobileController.php:533,536-545,615-632] Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
s.esmaeili
co-authored by
Claude Opus 5
Claude Opus 5
parent
4efe543d62
commit
024e3dd045
@@ -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.
|
||||
|
||||
@@ -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. |
|
||||
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user