diff --git a/apps/api/scripts/auth-smoke.sh b/apps/api/scripts/auth-smoke.sh new file mode 100644 index 0000000..2208bff --- /dev/null +++ b/apps/api/scripts/auth-smoke.sh @@ -0,0 +1,96 @@ +#!/usr/bin/env bash +# +# Auth smoke test for the routes closed in the security hotfix. +# +# For every route that was made non-public, asserts: +# - no token -> 401 +# - low-privilege token -> 403 +# - super-admin token -> anything but 401/403 (the guard let it through) +# +# And asserts the genuinely-public routes still answer 200, plus that +# GET /customers no longer leaks anything beyond the four login-lookup fields. +# +# Usage: API=http://localhost:3000/api bash scripts/auth-smoke.sh +# Needs the API running and the dev seed applied (npx prisma db seed). + +set -uo pipefail +API="${API:-http://localhost:3000/api}" + +SUPER_PHONE=09128081392; SUPER_PASS=09128081392 # ut 85, all rids +LOW_PHONE=09123334444; LOW_PASS=admin1234 # ut 88, 5 unrelated rids + +pass=0; fail=0 +ok() { printf ' \033[32mPASS\033[0m %s\n' "$1"; pass=$((pass+1)); } +bad() { printf ' \033[31mFAIL\033[0m %s\n' "$1"; fail=$((fail+1)); } + +login() { + curl -s -X POST "$API/admin/auth/login" -H 'Content-Type: application/json' \ + -d "{\"phone\":\"$1\",\"password\":\"$2\"}" | + sed -n 's/.*"api_token":"\([^"]*\)".*/\1/p' +} + +code() { # method path [token] + local m=$1 p=$2 t=${3:-} + if [ -n "$t" ]; then + curl -s -o /dev/null -w '%{http_code}' -X "$m" "$API/$p" \ + -H "Authorization: Bearer $t" -H 'Content-Type: application/json' -d '{}' + else + curl -s -o /dev/null -w '%{http_code}' -X "$m" "$API/$p" \ + -H 'Content-Type: application/json' -d '{}' + fi +} + +SUPER=$(login "$SUPER_PHONE" "$SUPER_PASS") +LOW=$(login "$LOW_PHONE" "$LOW_PASS") +[ -z "$SUPER" ] && { echo "super-admin login failed — is the seed applied?"; exit 1; } +[ -z "$LOW" ] && { echo "low-priv login failed — is the seed applied?"; exit 1; } + +# route | required rid +CLOSED=( + "GET|accounting/customers/x/balance|wallet.list" + "POST|accounting/customers/x/reconcile|wallet.charge" + "POST|accounting/repayments/gold|document.create" + "POST|accounting/repayments/rial|document.create" + "POST|accounting/entries/x/confirm|document.list.confirm" + "PATCH|orders/x/status|order.list.sts.submit" +) + +echo "Closed routes — no token must be 401:" +for row in "${CLOSED[@]}"; do + IFS='|' read -r m p _ <<<"$row" + c=$(code "$m" "$p") + [ "$c" = 401 ] && ok "$m $p -> 401" || bad "$m $p -> $c (want 401)" +done + +echo "Closed routes — low-privilege token must be 403:" +for row in "${CLOSED[@]}"; do + IFS='|' read -r m p rid <<<"$row" + c=$(code "$m" "$p" "$LOW") + [ "$c" = 403 ] && ok "$m $p [$rid] -> 403" || bad "$m $p [$rid] -> $c (want 403)" +done + +echo "Closed routes — super admin must get past the guard:" +for row in "${CLOSED[@]}"; do + IFS='|' read -r m p _ <<<"$row" + c=$(code "$m" "$p" "$SUPER") + case "$c" in + 401|403) bad "$m $p -> $c (guard rejected a super admin)" ;; + *) ok "$m $p -> $c (through the guard)" ;; + esac +done + +echo "Public routes must stay reachable:" +for p in "catalog/products" "pricing/latest?karatPerMille=750" "home/slider" "customers"; do + c=$(curl -s -o /dev/null -w '%{http_code}' "$API/$p") + [ "$c" = 200 ] && ok "GET $p -> 200" || bad "GET $p -> $c (want 200)" +done + +echo "GET /customers must expose only the login-lookup fields:" +keys=$(curl -s "$API/customers" | sed -n 's/.*\({[^}]*}\).*/\1/p' | head -1 | + grep -oE '"[a-zA-Z]+":' | tr -d '":' | sort -u | tr '\n' ' ') +want="id isActive isArchived mobile " +[ "$keys" = "$want" ] && ok "fields = $keys" || bad "fields = [$keys] want [$want]" + +echo +echo "passed: $pass failed: $fail" +[ "$fail" -eq 0 ] diff --git a/apps/api/src/modules/accounting/accounting.controller.ts b/apps/api/src/modules/accounting/accounting.controller.ts index b6286e7..86692a8 100644 --- a/apps/api/src/modules/accounting/accounting.controller.ts +++ b/apps/api/src/modules/accounting/accounting.controller.ts @@ -2,34 +2,53 @@ import { Body, Controller, Get, Param, Post } from '@nestjs/common'; import { LedgerService } from './ledger.service'; import { PostGoldRepaymentDto } from './dto/post-gold-repayment.dto'; import { PostRialRepaymentDto } from './dto/post-rial-repayment.dto'; -import { Public } from '../auth/guards'; +import { RequirePermission } from '../auth/guards'; -@Public() // pre-existing unauthenticated endpoint; see Phase 1 notes +/** + * Ledger operations — admin only. + * + * These were `@Public()` until the security hotfix. That was faithful to the + * reference, where the whole admin route file is unauthenticated (the + * `auth:api` group wrapper is commented out at henza_server/routes/admin.php:12 + * and :272), but it contradicted decision D1 in docs/parity/DECISIONS.md, which + * confirms the target keeps its stricter auth. Four of these five routes WRITE + * to the ledger, so an anonymous caller could reconcile a balance or post and + * confirm a repayment. + * + * Rids follow the legacy vocabulary: reading a balance is a wallet read, + * repayments are financial documents, and confirming an entry is the existing + * document-confirm permission. + */ @Controller('accounting') export class AccountingController { constructor(private readonly ledger: LedgerService) {} @Get('customers/:customerId/balance') + @RequirePermission('wallet.list') getBalance(@Param('customerId') customerId: string) { return this.ledger.getBalance(customerId); } @Post('customers/:customerId/reconcile') + @RequirePermission('wallet.charge') reconcile(@Param('customerId') customerId: string) { return this.ledger.reconcileBalance(customerId); } @Post('repayments/gold') + @RequirePermission('document.create') postGoldRepayment(@Body() dto: PostGoldRepaymentDto) { return this.ledger.postGoldRepayment(dto); } @Post('repayments/rial') + @RequirePermission('document.create') postRialRepayment(@Body() dto: PostRialRepaymentDto) { return this.ledger.postRialRepayment(dto); } @Post('entries/:entryId/confirm') + @RequirePermission('document.list.confirm') confirmEntry(@Param('entryId') entryId: string) { return this.ledger.confirmEntry(entryId); } diff --git a/apps/api/src/modules/customers/customers.controller.ts b/apps/api/src/modules/customers/customers.controller.ts index 9e42b92..fafc4b6 100644 --- a/apps/api/src/modules/customers/customers.controller.ts +++ b/apps/api/src/modules/customers/customers.controller.ts @@ -2,11 +2,31 @@ import { Controller, Get, Param } from '@nestjs/common'; import { CustomersService } from './customers.service'; import { Public } from '../auth/guards'; -@Public() // pre-existing unauthenticated endpoint; see Phase 1 notes +/** + * Customer-facing reads. + * + * Still `@Public()` because the target has no customer session yet — every one + * of these is called with `session.customerId` straight from the browser + * (DashboardView / ProfileView / OrdersView / TransactionsView), so any caller + * who guesses or harvests an id can read that customer's data. That is a + * textbook IDOR and it is what stage 2 of the security hotfix closes, by + * deriving the customer from a `customer-jwt` token instead of the URL + * (decision T1 in docs/parity/DECISIONS.md). The reference did exactly that + * via its `api-customer` guard. + * + * What stage 1 already fixed: `list()` no longer returns the full customer + * record — see LOGIN_LOOKUP_SELECT in customers.service.ts (SEC-08). + */ +@Public() // pre-existing unauthenticated endpoint; closed in hotfix stage 2 @Controller('customers') export class CustomersController { constructor(private readonly customers: CustomersService) {} + /** + * Login lookup only. Returns id/mobile/isActive/isArchived and nothing else; + * both apps' LoginView downloads this and matches the phone in the browser. + * Removed entirely in stage 2 once OTP verify returns the one customer. + */ @Get() list() { return this.customers.list(); diff --git a/apps/api/src/modules/customers/customers.service.ts b/apps/api/src/modules/customers/customers.service.ts index 025677c..e29099a 100644 --- a/apps/api/src/modules/customers/customers.service.ts +++ b/apps/api/src/modules/customers/customers.service.ts @@ -23,6 +23,31 @@ const PUBLIC_CUSTOMER_SELECT = { updatedAt: true, } satisfies Prisma.CustomerSelect; +/** + * The only fields `GET /customers` may expose. + * + * That route is unauthenticated because both user apps' login screens download + * the list and match the phone number in the browser. With the full select + * above, an anonymous caller got the entire customer book: every national ID + * number, every debt and credit limit, and the URLs of every identity-document + * scan. Logged as SEC-08 in docs/parity/ISSUES.md. + * + * The reference never allowed this — its login was + * POST userAuth (SMS) -> POST userVerfiy, and the server returned exactly ONE + * customer after the code matched (henza_server ApiMobileController.php:39-78). + * + * Stage 2 of the security hotfix restores that flow and removes this route from + * the public surface entirely. Until then these four fields are what the login + * screens actually read (LoginView.vue:131-133 matches on mobile/isActive/ + * isArchived, then calls session.login(id, mobile)) — nothing more. + */ +const LOGIN_LOOKUP_SELECT = { + id: true, + mobile: true, + isActive: true, + isArchived: true, +} satisfies Prisma.CustomerSelect; + @Injectable() export class CustomersService { constructor(private readonly prisma: PrismaService) {} @@ -46,7 +71,7 @@ export class CustomersService { list() { return this.prisma.customer.findMany({ where: { isActive: true, isArchived: false }, - select: PUBLIC_CUSTOMER_SELECT, + select: LOGIN_LOOKUP_SELECT, orderBy: { createdAt: 'desc' }, }); } diff --git a/apps/api/src/modules/orders/orders.controller.ts b/apps/api/src/modules/orders/orders.controller.ts index afb2937..a710e69 100644 --- a/apps/api/src/modules/orders/orders.controller.ts +++ b/apps/api/src/modules/orders/orders.controller.ts @@ -2,24 +2,42 @@ 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 } from '../auth/guards'; +import { Public, RequirePermission } from '../auth/guards'; -@Public() // pre-existing unauthenticated endpoint; see Phase 1 notes +/** + * Customer-facing 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. + */ @Controller('orders') export class OrdersController { constructor(private readonly orders: OrdersService) {} + @Public() @Post() create(@Body() dto: CreateOrderDto) { return this.orders.createOrder(dto); } + @Public() @Get(':id') get(@Param('id') id: string) { return this.orders.getById(id); } @Patch(':id/status') + @RequirePermission('order.list.sts.submit') updateStatus(@Param('id') id: string, @Body() dto: UpdateOrderStatusDto) { return this.orders.updateStatus(id, dto.status, dto.actorAdminUserId, dto.note); } diff --git a/apps/user-app-v2/src/api/client.ts b/apps/user-app-v2/src/api/client.ts index 3d0e633..27d0fec 100644 --- a/apps/user-app-v2/src/api/client.ts +++ b/apps/user-app-v2/src/api/client.ts @@ -70,13 +70,20 @@ export interface LedgerEntrySummary { note: string | null; } +/** + * What `GET /customers` returns — the login lookup, and nothing more. + * + * It used to carry fullName, national ID, both debts and the credit limit. The + * route is unauthenticated (the login screen matches the phone number in the + * browser), so that shipped the whole customer book to any visitor; the API now + * selects only these four fields. See SEC-08 in docs/parity/ISSUES.md. + * + * The route disappears altogether once OTP verify returns the one authenticated + * customer, the way the reference did it. + */ export interface CustomerSummary { id: string; - fullName: string; mobile: string; - goldDebtGrams: string; - fiatDebtRial: string; - goldCreditLimitGrams: string; isActive: boolean; isArchived: boolean; } diff --git a/apps/user-app/src/api/client.ts b/apps/user-app/src/api/client.ts index 443c641..617c7c2 100644 --- a/apps/user-app/src/api/client.ts +++ b/apps/user-app/src/api/client.ts @@ -85,13 +85,20 @@ export interface LedgerEntrySummary { note: string | null; } +/** + * What `GET /customers` returns — the login lookup, and nothing more. + * + * It used to carry fullName, national ID, both debts and the credit limit. The + * route is unauthenticated (the login screen matches the phone number in the + * browser), so that shipped the whole customer book to any visitor; the API now + * selects only these four fields. See SEC-08 in docs/parity/ISSUES.md. + * + * The route disappears altogether once OTP verify returns the one authenticated + * customer, the way the reference did it. + */ export interface CustomerSummary { id: string; - fullName: string; mobile: string; - goldDebtGrams: string; - fiatDebtRial: string; - goldCreditLimitGrams: string; isActive: boolean; isArchived: boolean; } diff --git a/docs/parity/CHANGELOG.md b/docs/parity/CHANGELOG.md index 46d3e7e..463acd4 100644 --- a/docs/parity/CHANGELOG.md +++ b/docs/parity/CHANGELOG.md @@ -27,3 +27,68 @@ Format: - **Matrix row:** n/a _No source code changed in Phase 0 (read-only phase)._ + +--- + +## 2026-09-22 — Security hotfix, stage 1 — close the unauthenticated write paths + +Branch `parity/security-hotfix`. Authorised by the user's rulings D7, T1–T3. + +### 1. Ledger routes are admin-only (SEC-09) + +- **What:** removed the controller-level `@Public()` from `AccountingController` + and gave each route a rid: `wallet.list` (balance), `wallet.charge` + (reconcile), `document.create` (both repayments), `document.list.confirm` + (confirm entry). +- **Why:** four of the five routes WRITE to the ledger and were reachable with no + token at all. Faithful to the reference, whose entire admin route file is + unauthenticated, but contrary to confirmed decision D1. +- **Reference:** `henza_server/routes/admin.php:12,272` (the `auth:api` wrapper is + commented out at both ends) +- **Target:** `apps/api/src/modules/accounting/accounting.controller.ts` +- **Breakage check:** `api.getBalance()` is the only app-side consumer and it has + **zero call sites** in either user app (dead code, already logged as DEAD-01/02). + +### 2. Order status change is admin-only (SEC-10) + +- **What:** moved `@Public()` from the class to the two customer-facing handlers, + leaving `PATCH orders/:id/status` behind `order.list.sts.submit`. +- **Why:** it let an anonymous caller move any order's status **and** attribute + the change to any admin through the DTO's `actorAdminUserId`. +- **Reference:** an admin operation; the admin panel posts to `admin/orders/status` + (`apps/admin-panel/src/api/endpoints.ts:42,48`), so this route had no caller. +- **Target:** `apps/api/src/modules/orders/orders.controller.ts` +- **Still public:** `POST /orders` and `GET /orders/:id`, because the target has + no customer session yet — closed in stage 2. + +### 3. `GET /customers` no longer leaks the customer book (SEC-08) + +- **What:** added `LOGIN_LOOKUP_SELECT` (`id`, `mobile`, `isActive`, + `isArchived`) and pointed `list()` at it. Removed `fullName`, `nationalCode`, + `goldDebtGrams`, `fiatDebtRial`, `goldCreditLimitGrams`, `verificationStep`, + `cityId`, `referredByAgentId`, `nationalIdImageUrl`, `businessLicenseUrl`, + `shopImageUrl`, `createdAt`, `updatedAt` from that response. +- **Why:** the route is unauthenticated because both apps match the phone number + in the browser, so it shipped every customer's national ID number, debts, + credit limit and identity-document URLs to any visitor. +- **Reference:** login was `POST userAuth` → SMS → `POST userVerfiy`, returning + exactly one customer after the code matched + (`henza_server/app/Http/Controllers/Api/ApiMobileController.php:39-78`). +- **Target:** `apps/api/src/modules/customers/customers.service.ts`, + plus `CustomerSummary` narrowed in **both** apps' `src/api/client.ts` + (parity rule: the same logic change in both). +- **Route survives** until stage 2 replaces the client-side phone match with OTP. + +### 4. Test script + +- **What:** added `apps/api/scripts/auth-smoke.sh` — asserts 401 without a token, + 403 with a low-privilege token, and pass-through for a super admin, on every + closed route; plus that the public routes still answer 200 and that + `GET /customers` exposes only the four login fields. +- **Result:** **23 passed, 0 failed** against a live server and the dev seed. + +### Not changed + +`/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). diff --git a/docs/parity/DECISIONS.md b/docs/parity/DECISIONS.md index 09c1273..89aab1f 100644 --- a/docs/parity/DECISIONS.md +++ b/docs/parity/DECISIONS.md @@ -166,6 +166,67 @@ Run it for the sales expert too. --- +## T1 — Customer authentication: a separate JWT strategy (option **الف**) + +Approved. Conditions: + +1. **Separate secrets** — customer tokens are signed with `JWT_CUSTOMER_SECRET`, + admin tokens with `JWT_SECRET`. +2. **A `typ` claim is checked in both guards** — `typ: 'admin'` vs `typ: 'customer'`. +3. **Mandatory cross-token tests:** a customer token on any admin route → 401; + an admin token on a customer route → rejected, unless deliberately allowed. +4. **Token lifetime and logout must be compared against the reference and + recorded here** (see below). +5. **No IDOR** — customer routes that return personal data derive the customer + from the **token**, never from an id in the request. + +### Reference comparison — token lifetime and logout + +| Aspect | Reference | Target (current) | Decision | +|---|---|---|---| +| Customer token | `api_token = Str::random(60)`, a random opaque string stored on the customer row (`ApiMobileController.php:60-62`) | none | new customer JWT | +| Expiry | **none** — the reference token never expires; it is reused on every login if already set | admin JWT `JWT_EXPIRES_IN`, default `12h` | customer JWT gets an explicit expiry (deliberate improvement over the reference) | +| Logout | the reference has **no logout endpoint**; the app drops the token client-side | apps clear `localStorage` | keep client-side clear; note that a stolen token stays valid until expiry | +| Rotation | the reference issues a new token **only if `api_token` is null** — logging in twice returns the same token forever | n/a | issue a fresh token on every verify | + +The last three rows are **deliberate improvements over the reference**, approved +by the user in the same message that approved D6. Recorded so the matrix does not +later flag them as parity gaps. + +## T2 — Uploaded files stay public + +**Ruling:** `/uploads` is not touched. No allow-list, no signed URLs, and the +"private folder" stage is dropped. + +**Recorded verbatim at the user's request:** +> «فایل‌های آپلودی شامل مدارک عمداً عمومی هستند — تصمیم مالک پروژه» +> (Uploaded files, including identity documents, are deliberately public — the +> project owner's decision.) + +### The light check the user asked for — result: ✅ already correct + +| Check | Result | Evidence | +|---|---|---| +| Filenames unguessable? | **Yes.** `md5(Date.now() + originalname + randomUUID())` + the original extension | `src/modules/admin-uploads/admin-uploads.service.ts:53` | +| Sequential or original name on disk? | **No.** `originalName` is kept only as a DB column, never as the filename | `admin-uploads.service.ts:62` | +| Directory listing? | **No.** `PublicUploadsController` only resolves a single `:filename`, and `resolveStoredUpload()` rejects `.`/`..` and anything resolving outside the upload dir | `public-uploads.controller.ts:17`, `upload-path.ts:34-49` | + +Nothing to change. No existing files were moved. + +## T3 — Two-stage hotfix + +Approved, with the condition that `GET /customers` be checked first. + +**Check result:** the apps **are** dependent on it. Its only consumer is +`LoginView.vue:131` in both apps, and `CustomerSummary` is used nowhere else +(verified by grep). The fields actually read are `mobile`, `isActive`, +`isArchived`, then `session.login(id, mobile)` — four fields. + +So per the ruling: stage 1 strips the sensitive fields and keeps the route; stage +2 removes the route when login moves to OTP. + +--- + # Urgent investigation results ## `percent` — full trace (D9) diff --git a/docs/parity/ISSUES.md b/docs/parity/ISSUES.md index 0f723b7..782421d 100644 --- a/docs/parity/ISSUES.md +++ b/docs/parity/ISSUES.md @@ -18,9 +18,9 @@ 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 | 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 | Fixed as a side-effect of D6: once OTP verify returns the authenticated customer, take `GET /customers` off the public surface. | -| SEC-09 | Critical | 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 | Put behind the admin guard with the matching wallet/document rid — **awaiting user approval** | -| SEC-10 | High | 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 | Protect with the order-status rid | +| 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-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. | | SEC-05 | High | TGT `apps/api/prisma/seed.ts` | Seed creates a SUPER_ADMIN (`09128081392`) whose **password equals the phone number**, holding all 144 rids. Marked DEV-ONLY in comments but lives in the normal seed. | `docs/CHANGELOG-PORT-HISTORY.md` §4 documents this as intentional for dev. | Gate behind an explicit env flag so it can never run outside dev. | | SEC-06 | High | REF `henza_server` | Hardcoded secrets committed to the repo: SMS panel password, PayPing bearer token, Tahesab bearer + IP allowlist, wallet hash pepper (`"yhoenszeaf"`). | See `_inv-ref-server.md` §11.4. | Rotate all of them before any deployment that reuses these credentials. | diff --git a/docs/parity/PROGRESS.md b/docs/parity/PROGRESS.md index 5aa3681..a363be8 100644 --- a/docs/parity/PROGRESS.md +++ b/docs/parity/PROGRESS.md @@ -7,12 +7,27 @@ | Field | Value | |---|---| -| **Phase** | Phase 0 ✅ passed → **Phase 1 starting** | -| **Branch** | `parity/phase-0` (next: `parity/phase-1-admin`) | -| **Status** | Gate passed. Decisions D0–D12 recorded in `DECISIONS.md`. Urgent traces (percent, @Public, login leak) done and delivered. | -| **Next step** | Show the user the `percent` trace + `@Public` list (done), get approval on (a) protecting the 5 ledger routes and (b) the `percent` schema change → then build `01-ADMIN-MATRIX.md` **field-by-field** (D12) | -| **Blocked on** | user approval for: the 5 `@Public` ledger routes (D7), the `percent` column (D9), the D0 migration file (approval required before running) | -| **Last updated** | 2026-09-21 | +| **Phase** | Security hotfix (out-of-band, authorised by D7/T1–T3) — **stage 1 ✅ done**, stage 2 next | +| **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** | +| **Last updated** | 2026-09-22 | + +### Security hotfix — stage 1 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 | + +Test: `apps/api/scripts/auth-smoke.sh` — **23 passed, 0 failed**. +Typecheck: api ✅ · user-app ✅ · user-app-v2 ✅. ## ⚠️ Read `DECISIONS.md` before doing anything