s.esmaeili/tehran_taj
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

parity(api,apps): security hotfix stage 2 - real customer sessions, close IDOR

Browse Source 
Stage 1 narrowed GET /customers but did not close the leak: the response still
carried every customer id, and /customers/:id, /:id/orders and
/:id/ledger-entries were still public, so the full customer book was still two
requests away. The user caught this. Stage 2 deletes the id-bearing surface.

Customer JWT, separate from the admin one (T1):
  - new src/modules/customer-auth/ with its own passport strategy,
    CustomerJwtGuard, @CustomerAuth() and @CurrentCustomer()
  - signed with JWT_CUSTOMER_SECRET, and every token carries typ:'customer'
    which the strategy verifies; both checks are present deliberately
  - customer state re-read per request, so deactivating takes effect at once
  - mirrors the reference's own split: auth:api vs auth('api-customer')

Real OTP (D6), porting ApiMobileController::userAuth/userVerfiy (:39-64):
  - new CustomerOtp table, additive migration, nothing existing touched
  - parity behaviour: rand(1000,9999), active-customer filter, faTOen digit
    normalisation on both number and code, rejection on mismatch
  - approved improvements over the reference: bcrypt-hashed code, 2-minute
    expiry, 5 attempts then lockout, single use, send throttle (60s per
    number, 5/number and 20/IP per 10 min), and no number enumeration

Routes removed, replaced by token-derived equivalents:
  GET customers, customers/:id, /:id/orders, /:id/ledger-entries
    -> GET customer/me, customer/me/orders, customer/me/ledger-entries
No route accepts a customer id any more, so the IDOR is closed structurally
rather than by a check. The admin panel was unaffected (admin/customers-omde/*).

Both apps migrated in the same commit per the v1/v2 parity rule: token-based
session store, Bearer header, 401 -> clear session and route to login, real
two-step OTP login. Each app keeps its own markup and styling; only logic moved.

Tests: auth-smoke.sh extended with the stage-2 assertions, 46 passed 0 failed
against a live server. jest installed dev-only (user-approved) so pnpm test
works for the first time: 7 passed covering faToEn, which moved to its own
digits.ts to be testable without booting Nest. Typecheck clean on api,
user-app and user-app-v2; both apps build.

[ref: henza_server ApiMobileController.php:39-78]

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
s.esmaeili
2026-09-22 01:20:00 +03:30
co-authored by Claude Opus 5
parent fdf833565f
commit 4efe543d62
42 changed files with 3552 additions and 277 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/parity/CHANGELOG.md
+84
View File
@@ -92,3 +92,87 @@ Branch `parity/security-hotfix`. Authorised by the user's rulings D7, T1–T3.
`/uploads` — per ruling T2, uploaded files including identity documents stay
public. The filename check requested alongside it passed with no changes needed
(md5 of time + name + UUID; no directory listing).
---
## 2026-09-22 — Security hotfix, stage 2 — real customer sessions
Branch `parity/security-hotfix`. Authorised by D6, D7, T1 and T3.
Stage 1 narrowed `GET /customers` but, as the user pointed out, did not close
the leak: the response still carried every customer **id**, and
`/customers/:id`, `/:id/orders` and `/:id/ledger-entries` were still public — so
two requests still reconstructed the whole book. Stage 2 removes the id-bearing
surface entirely.
### 1. Customer JWT, separate from the admin one (T1)
- **New:** `src/modules/customer-auth/` — `CustomerJwtStrategy` (passport
strategy `'customer-jwt'`), `CustomerJwtGuard`, `@CustomerAuth()`,
`@CurrentCustomer()`.
- Signed with **`JWT_CUSTOMER_SECRET`**, separate from `JWT_SECRET`, and every
token carries **`typ: 'customer'`** which the strategy verifies. Either check
alone would be enough; both are present deliberately.
- Customer state is re-read from the DB on every request, so deactivating or
archiving a customer takes effect at once rather than at token expiry.
- **Reference:** the same split existed there — `auth:api` for admins,
`auth('api-customer')` for the app.
### 2. Real OTP (D6)
- **New table `CustomerOtp`** (migration `20260922000000_customer_otp`,
additive: one new table, nothing existing touched).
- `POST /api/customer/auth/request-otp` ← `ApiMobileController::userAuth` (:39-49)
- `POST /api/customer/auth/verify-otp` ← `ApiMobileController::userVerfiy` (:51-64)
- **Parity behaviour:** the 4-digit `rand(1000,9999)` range, the
active-customer filter, `Utility::faTOen()` digit normalisation on both the
number and the code, and rejection on mismatch.
- **Deliberate improvements over the reference** (user-approved, recorded in
DECISIONS.md so the matrix does not flag them later):
- the code is stored as a **bcrypt hash**, never plain text;
- it **expires** after 2 minutes (the reference's never did);
- **5 attempts** per challenge, then lockout (the reference had no limit);
- **single use** — a consumed code cannot be replayed;
- **send throttle**: 60s cooldown per number, 5 per number and 20 per IP per
10 minutes, counted off the table so it survives restarts;
- **no enumeration** — an unregistered number returns the same `{sent:true}`.
The reference's `firstOrFail()` leaked a 404 for unknown numbers.
- No SMS gateway exists in this project (STUB-02), so the code is logged in
non-production only and the message is persisted as an `SmsMessage` row. The
reference's message text is kept verbatim for when a gateway lands.
### 3. The id-bearing customer routes are gone
- **Deleted** `customers.controller.ts` — with it `GET /customers`,
`/customers/:id`, `/:id/orders`, `/:id/ledger-entries`.
- **Replaced by** `/api/customer/me`, `/me/orders`, `/me/ledger-entries`, which
take the id **from the token**. There is no longer any route that accepts a
customer id, so the IDOR is closed structurally rather than by a check.
- `CustomersService` stays and is reached only through those routes.
- The admin panel was unaffected — it uses `admin/customers-omde/*`.
### 4. Both apps migrated together (parity rule)
- `stores/session.ts`: `token` is now the credential and `isLoggedIn` is
token-based. `customerId` is kept for display only and is no longer an
identity claim.
- `api/client.ts`: sends `Authorization: Bearer`, throws `UnauthorizedError` on
401 and calls the handler registered in `main.ts`, which clears the session
and routes to login so stale personal data cannot stay on screen.
- `LoginView.vue`: real two-step OTP. Each app keeps its own markup and styling;
only the logic changed, and it is identical in both.
- The four account screens now call `getMe()` / `getMyOrders()` /
`getMyLedgerEntries()`; router guards test `isLoggedIn`.
### Tests
- `apps/api/scripts/auth-smoke.sh` extended with the stage-2 assertions:
**46 passed, 0 failed** against a live server — old routes 404, customer
routes 401 without a token, wrong/reused code 401, correct code 200, throttle
429, no enumeration, customer token rejected on admin routes and admin token
rejected on customer routes, no id-bearing surface, and `/customer/me`
returning the token's own customer.
- **jest installed** (dev-only, user-approved) with `jest.config.cjs`, so
`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.
docs/parity/DECISIONS.md
+38
View File
@@ -421,3 +421,41 @@ and returned **only that one customer** after the code matched. There was no
lookup. **D6 (real OTP) is the fix for both problems at once** — once
`POST auth/otp/verify` returns the authenticated customer, `GET /customers` can
be taken off the public surface entirely.
---
# Stage 2 implementation record (2026-09-22)
## Deliberate improvements over the reference — user-approved
Recorded here so the Phase 1/2 matrices do not later flag them as parity gaps.
| # | Behaviour | Reference | Target | Approved in |
|---|---|---|---|---|
| 1 | OTP storage | plain text in `customer.send_password` | bcrypt hash in `CustomerOtp.codeHash` | D6 |
| 2 | OTP expiry | none — valid forever | 2 minutes | D6 |
| 3 | Attempt limit | none | 5 per challenge, then lockout | D6 |
| 4 | Replay | code stayed usable | single use (`consumedAt`) | D6 |
| 5 | Send throttle | none | 60s cooldown/number, 5 per number and 20 per IP per 10 min | user message of 2026-09-22 |
| 6 | Unknown number | `firstOrFail()` → 404, enumerable | identical `{sent:true}` | implied by 5; recorded here |
| 7 | Token rotation | `api_token` issued once, reused forever | fresh token on every verify | T1 |
| 8 | Token expiry | none | `JWT_CUSTOMER_EXPIRES_IN`, default 30d | T1 |
| 9 | Token secret | single opaque string | `JWT_CUSTOMER_SECRET`, separate from admin, plus a `typ` claim | T1 |
## New environment variables
| Name | Default | Purpose |
|---|---|---|
| `JWT_CUSTOMER_SECRET` | falls back to `JWT_SECRET`, then a dev string | signs customer tokens |
| `JWT_CUSTOMER_EXPIRES_IN` | `30d` | customer token lifetime |
Both fall back so local development keeps working, and both inherit the SEC-04
problem (an insecure default when unset) — tracked there, not re-logged.
## Known gap left open on purpose
`POST /orders` and `GET /orders/:id` are still `@Public()`. The customer token
now exists, so they can be closed, but `CreateOrderDto` carries `customerId` in
the body and changing that is an API-contract change touching both apps' cart
flow. Left for the Phase 2 apps work rather than widened into this hotfix.
Tracked as SEC-02 (B1/B2) in `ISSUES.md`.
docs/parity/ISSUES.md
+2 -2
View File
@@ -17,8 +17,8 @@ Severity: `Critical` (money loss / security / data loss) · `High` · `Medium`
|---|---|---|---|---|---|
| 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-03 | Critical | 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 | **FIXING — D6** (plus expiry + attempt limits, which exceed the reference) |
| SEC-08 | ~~Critical~~ **MITIGATED — hotfix stage 1** | 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 | ✅ Stage 1: narrowed to `id`/`mobile`/`isActive`/`isArchived`. National ID, debts, credit limit and document URLs no longer leave the API. Route closes in stage 2 with OTP. |
| 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`. |
| SEC-10 | ~~High~~ **FIXED — hotfix stage 1** | TGT `apps/api/src/modules/orders/orders.controller.ts:22` | `PATCH orders/:id/status` is public **and** its DTO accepts a client-supplied `actorAdminUserId`, so an anonymous caller can move an order's status and attribute it to any admin. In the reference this was an admin operation. | `DECISIONS.md` §"@Public audit" B3 | ✅ Done: `order.list.sts.submit`. Had no caller — the admin panel uses `admin/orders/status`. |
| SEC-04 | High | TGT `apps/api` (two files) | `JWT_SECRET` falls back to `'dev-only-insecure-secret'` and is absent from `.env.example`. `enableCors()` is wide open. | Reported by api inventory; see `_inv-tgt-api.md` §11. | Require the env var at boot; restrict CORS origins. |
docs/parity/PROGRESS.md
+36 -16
View File
@@ -7,27 +7,47 @@
| Field | Value |
|---|---|
| **Phase** | Security hotfix (out-of-band, authorised by D7/T1–T3) — **stage 1 ✅ done**, stage 2 next |
| **Phase** | Security hotfix — **stages 1 and 2 ✅ complete** |
| **Branch** | `parity/security-hotfix` |
| **Status** | Stage 1 complete and tested live: 23/23 smoke assertions pass. Decisions D0–D12 + T1–T3 recorded. |
| **Next step** | Stage 2 — customer JWT (`typ` claim, separate secret) + real OTP + migrate both apps' login; then remove `GET /customers` from the public surface |
| **Blocked on** | nothing for stage 2. Still pending later: D0 migration + `percent` migration must be **shown and approved before running** |
| **Status** | Both stages done and verified live: **46/46** smoke assertions, **7/7** jest unit tests, typecheck + build clean on all three projects. |
| **Next step** | Show the user the **D0 (firstName/lastName)** and **`percent`** migrations for approval, then start `01-ADMIN-MATRIX.md` field-by-field (D12) |
| **Blocked on** | user approval of those two migrations before they run |
| **Last updated** | 2026-09-22 |
### Security hotfix — stage 1 result
### Security hotfix — final result
| Route | Was | Now | Rid |
|---|---|---|---|
| `GET accounting/customers/:id/balance` | public | admin | `wallet.list` |
| `POST accounting/customers/:id/reconcile` | public **write** | admin | `wallet.charge` |
| `POST accounting/repayments/gold` | public **write** | admin | `document.create` |
| `POST accounting/repayments/rial` | public **write** | admin | `document.create` |
| `POST accounting/entries/:id/confirm` | public **write** | admin | `document.list.confirm` |
| `PATCH orders/:id/status` | public **write** | admin | `order.list.sts.submit` |
| `GET customers` | 18 fields incl. national ID, debts, doc URLs | 4 fields | still public until stage 2 |
| Route | Was | Now |
|---|---|---|
| `GET accounting/customers/:id/balance` | public | admin, rid `wallet.list` |
| `POST accounting/customers/:id/reconcile` | public **write** | admin, rid `wallet.charge` |
| `POST accounting/repayments/gold` | public **write** | admin, rid `document.create` |
| `POST accounting/repayments/rial` | public **write** | admin, rid `document.create` |
| `POST accounting/entries/:id/confirm` | public **write** | admin, rid `document.list.confirm` |
| `PATCH orders/:id/status` | public **write** | admin, rid `order.list.sts.submit` |
| `GET customers` | every customer, 18 fields | **route deleted** |
| `GET customers/:id` | public | **route deleted** → `GET customer/me` (token) |
| `GET customers/:id/orders` | public | **route deleted** → `GET customer/me/orders` (token) |
| `GET customers/:id/ledger-entries` | public | **route deleted** → `GET customer/me/ledger-entries` (token) |
| — | — | **new** `POST customer/auth/request-otp` · `verify-otp` |
Test: `apps/api/scripts/auth-smoke.sh` — **23 passed, 0 failed**.
Typecheck: api ✅ · user-app ✅ · user-app-v2 ✅.
Still public by design: `catalog/products`, `pricing/latest`, `home/slider`,
`uploads/:filename` (ruling T2), and `POST /orders` + `GET /orders/:id` — the
last two are logged as SEC-02/B1-B2 and want a customer-token pass of their own.
**Tests**
- `apps/api/scripts/auth-smoke.sh` (live server): **46 passed, 0 failed**.
Run it with `API=http://localhost:3000/api CODE_LOG=/path/to/api.log`.
- `pnpm test` in `apps/api` (jest, newly installed): **7 passed**.
- Typecheck: api ✅ · user-app ✅ · user-app-v2 ✅. Both apps build ✅.
### ⚠️ Correction worth remembering
Stage 1 reported the customer-book leak as "mitigated" after narrowing the
`GET /customers` field list. That was wrong: the response still returned every
customer **id**, and the three `/customers/:id*` routes were still public, so the
full data was still two requests away. The user caught it. Stage 2 deleted the
whole id-bearing surface instead. **Narrowing a response is not closing a leak
while the id and the id-addressed routes remain.**
## ⚠️ Read `DECISIONS.md` before doing anything