s.esmaeiliandClaude Opus 5
654b98520d
Prisma migrate diff proposed DROP TABLE "_goldbank_migrations" because the table exists in the database but was not declared in the schema. Declaring it with @@ignore removes the proposal without exposing it to Prisma Client, and the table and all its rows are kept. Correcting the earlier DEAD-07 note: this is not a leftover from the pre-Prisma port. It is the tracking table of the application's own migration runner (src/prisma/migrate.ts:5), which exists because the Docker deploy image is copy-only and ships no Prisma CLI, so migrate deploy cannot run in the container (migrate.ts:7-13). The project therefore keeps two ledgers deliberately: _prisma_migrations for the CLI in development, and _goldbank_migrations for the runtime runner in the container. That runner already filtered out any DROP of its own table (migrate.ts:190-196); the schema declaration removes the cause rather than relying on that guard. The practical consequence, recorded in DECISIONS.md D15: a new migration must be safe under both paths, so its SQL stays idempotent. The ledger grew 12 -> 14 rows while testing, with nothing deleted: starting the API ran prepareDatabase(), which appended the two migrations applied earlier today. Its $$-aware statement splitter handled the PL/pgSQL trigger body correctly — trigger and function are both present and the 9 trigger assertions pass against the live database. Also records the UTF-8 testing rule: Persian test input must come from a file, never a shell argument, because the Windows shell replaces Persian characters with '?' and a test can then pass while storing '????'. Verified: migrate diff -> "This is an empty migration" (zero drift); migrate status -> up to date; tsc clean; 32 jest; 9 trigger assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
121 lines
20 KiB
Markdown
121 lines
20 KiB
Markdown
# ISSUES — logged, NOT fixed during parity
|
||
|
||
Anything found during the parity audit that is a bug, risk, smell or improvement
|
||
opportunity goes here. **Do not fix these while parity work is in progress** (see
|
||
`CLAUDE.md` §3). They are worked through in Phase 3, only when the user asks.
|
||
|
||
Severity: `Critical` (money loss / security / data loss) · `High` · `Medium` · `Low`
|
||
|
||
> Paths are relative to the workspace root (`gold bank/`).
|
||
> "REF" = reference project (read-only). "TGT" = `gold-banking-monorepo`.
|
||
> Items marked **[replicated]** exist in the reference and were faithfully ported —
|
||
> fixing them is a product decision, not a parity fix.
|
||
|
||
## Security & auth
|
||
|
||
| 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~~ **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`. |
|
||
| 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. |
|
||
| SEC-07 | Medium | TGT `apps/api/src/modules/admin-customers/admin-customers.service.ts` | `setRoles` stores arbitrary strings as rids with no validation against `ALL_RIDS`. | api inventory §9. | Validate against the rid catalogue. |
|
||
|
||
## Customer / person module (found during the user-requested deep check)
|
||
|
||
> These are **parity defects being fixed**, not deferred items — see
|
||
> `DECISIONS.md` D0, D9, D10, D11. Listed here so the trail is complete.
|
||
|
||
| ID | Severity | Where | Description | Evidence | Status |
|
||
|---|---|---|---|---|---|
|
||
| CUST-01 | Critical | TGT `apps/api/src/modules/admin-customers/admin-customers.service.ts:231-233` | **First and last name collapse.** The reference stores `fname`/`lname` as two columns; the target merged them into `fullName` and returns `fname: c.fullName, lname: ''`. Opening any edit form puts the whole name in the first-name box and blanks the surname; saving then persists that. `singleCustomer.vue` tries to compensate with `split(' ')`, which mis-splits multi-part names. | REF `database/migrations/2020_10_01_120430_customers_table.php:18-19` (two columns) vs TGT `:231-233`. Reported by the user from live use. | **FIXING — D0** |
|
||
| CUST-02 | Critical | TGT same file `:245`, `:600-658` | **`percent` read from the wrong entity and never written.** Target returns `c.referredByAgent?.wagePercent`; the reference reads the customer's **own** `percent` column. `update()` ignores `data.percent` entirely, so editing the field does nothing. The create-time default (3) and the admin-path `required` rule are both missing, as is the app-side `filterPercentPay` offset. | REF `database/migrations/2020_10_19_153228_customer_add.php:32`; `AdminController.php:338,354`; `ApiMobileController.php:391-396,76-77`. Full trace in `DECISIONS.md` §percent. | **FIXING — D9** |
|
||
| CUST-03 | High | TGT `apps/admin-panel/src/views/apps/customer/ReagentCustomer.vue:97-110` | **Sales-expert form is missing 5 reference fields:** `ncode`, `phone`, `city_id`, `gram_level`, `reagent_id`. The reference form had 15; the target has 8. Likely because `AdminUser` lacks those columns. | REF `henza_admin/src/views/apps/customer/ReagentCustomer.vue` (15 `v-decorator` fields) vs TGT form object. | **INVESTIGATING — D10** |
|
||
| CUST-04 | Low | TGT `apps/admin-panel/src/views/apps/customer/singleCustomer.vue:91` | Still spreads the whole form on save while the other two customer forms send a diff. Harmless today (4 fields, all repopulated) but the same shape that caused the earlier data loss. | read at `:80-106` | **FIXING — D11** |
|
||
| CUST-05 | Low | TGT `apps/admin-panel/src/views/apps/customer/bulkCustomer.vue` | Diff-send on edit is a **deliberate deviation** from the reference, which posts all 15 fields on every save. Recorded so it is not "fixed" back. | `DECISIONS.md` D11 | accepted deviation |
|
||
|
||
## Money, stock & data integrity
|
||
|
||
| ID | Severity | Where | Description | Evidence | Suggested fix |
|
||
|---|---|---|---|---|---|
|
||
| MON-01 | Critical | TGT `apps/user-app-v2/src/stores/cartStore.ts` | v2 has **no `capQty` / `maxQty` stock ceiling**, which `user-app` does have (`apps/user-app/src/stores/cartStore.ts:62,65`; `components/product/CartItem.vue:82-83`). A v2 user can order more units than exist. | Verified by grep across both apps — zero hits in v2. | Port the ceiling to v2 (this is also a **Phase 2 parity gap**, row to be added to `02-APPS-MATRIX.md`). |
|
||
| MON-02 | High | REF `henza_server` wallet package | The wallet `balance` column **mixes grams and toman in one field**, and money operations are not wrapped in DB transactions. | `_inv-ref-server.md` §7. | Separate the units; wrap mutations in transactions. |
|
||
| MON-03 | High | TGT `apps/api/src/modules/accounting/ledger.service.ts:189` | `GRAMS_PER_MESGHAL = 4.331802` is a JS **float** used inside an otherwise-`Prisma.Decimal` CASH repayment formula, so the divisor is pre-rounded before the Decimal division. | Read at `ledger.service.ts:182-190`; the source comment already flags the formula for stakeholder review. | Make the constant a `Decimal` (or a string literal passed to `Decimal`). |
|
||
| MON-04 | High | TGT `apps/api/src/modules/orders/orders.service.ts` | Debt is recognised at `DELIVERED_TO_AGENT`, but the idempotency guard checks `deliveredAt`, which is only set by `DELIVERED_TO_CUSTOMER` — so the debt trigger is not reliably idempotent. There is also **no status-transition validation** anywhere. | api inventory §8. | Guard on the same field the trigger fires on; add an explicit transition table. |
|
||
| MON-05 | Medium | TGT `apps/api` | `canPlaceOrders` and `verificationStep` are stored and toggled by the admin panel but **never enforced** on any order path. | api inventory §8. | Enforce at order creation, or remove the toggles. |
|
||
| MON-06 | Medium | TGT `apps/api` | `Setting` rows are never seeded — `wallet.config`, `wallet.birthday`, `promotion.auto`, `mobile_main_slider` only materialise on first write, so a fresh install has undefined behaviour until an admin saves each one. | api inventory §7. | Seed the defaults. |
|
||
| MON-07 | Medium | REF `henza_server/app/Models/ProductVariant.php:183-190` vs `SingleCustomerOrders.php:67`, `Order.php:66` | Two **divergent order-status enums** coexist (0–6 and 1–7) for what is conceptually one flow. | Read both models. | Unify, or document the mapping explicitly. |
|
||
|
||
## Validation & contracts
|
||
|
||
| ID | Severity | Where | Description | Evidence | Suggested fix |
|
||
|---|---|---|---|---|---|
|
||
| VAL-01 | High | REF `henza_server` | **Zero FormRequests**; all validation is inline, and `setOrder`, wallet `charge`, `setConst` and file upload have **no validation at all**. The wallet package's own validator is dead code (it calls `failed()` instead of `fails()`). | `_inv-ref-server.md` §12. | n/a in reference; informs how much validation the target must add. |
|
||
| VAL-02 | High | TGT `apps/api` | Only **8 DTO classes** exist; most admin request bodies are untyped `Record<string, unknown>`, so class-validator never runs on them. | api inventory §5. | Add DTOs for the admin write paths. |
|
||
| VAL-03 | Medium | TGT both user apps | HTTP client is a bare `fetch` wrapper: **no interceptors, no Authorization header, no refresh, no 401 handling, no error toasts**. Non-2xx throws a generic `Error`; call sites mostly `console.log(e)`. Order-submit failure is **silent to the user**. | `apps/user-app/src/api/client.ts`, `apps/user-app-v2/src/api/client.ts:3`. | Add a shared client with error surfacing; compare against `henza_pwa/src/service/*` in Phase 2. |
|
||
|
||
## Parity-relevant divergence between user-app and user-app-v2
|
||
|
||
> `CLAUDE.md` §2: the only allowed difference is visual design. These are logic
|
||
> differences, so they are **Phase 2 parity gaps** as well as issues.
|
||
|
||
| ID | Severity | Where | Description | Evidence |
|
||
|---|---|---|---|---|
|
||
| V2-01 | Critical | v2 missing stock ceiling | see MON-01 | grep |
|
||
| V2-02 | Medium | v2 missing `getHomeSlider()` / `GET /home/slider` admin banner | v1 `api/client.ts:152-170` defines `AdminSlide` + `getHomeSlider`; v2 has neither | `diff apps/user-app/src/api/client.ts apps/user-app-v2/src/api/client.ts` |
|
||
| V2-03 | Medium | v2 missing per-variant images | v1 defines `ProductImageSummary` (`id/url/sortOrder/role`) and `imageId`/`image` on the variant; v2's `images` is just `Array<{url:string}>` | same diff |
|
||
| V2-04 | Medium | v2 missing orderable-first variant defaulting | v1 `composables/useProduct.ts:26` picks the first orderable variant; v2 has no equivalent | grep |
|
||
| V2-05 | Low | v2 missing variant-chip preview cap + per-chip stock badges | v1 `components/product/SingleProduct.vue:388-392` | grep |
|
||
|
||
## Dead code & maintenance
|
||
|
||
| ID | Severity | Where | Description |
|
||
|---|---|---|---|
|
||
| DEAD-01 | Low | TGT `apps/user-app` | 8 components with no importers, `loadingStore`, `api.getBalance` (zero call sites), unused `gsap` dependency. |
|
||
| DEAD-02 | Low | TGT `apps/user-app-v2` | 11 dead components, `loadingStore`, `getBalance`, `priceUnit`/`priceUnitRialLess`, 12 of 15 validators, unused `gsap` + `@ant-design/icons-vue` deps. |
|
||
| DEAD-03 | Low | TGT `gold-banking-monorepo/user-app-v2/` (repo root, outside `apps/`) | Stray leftover folder containing only `package.json` and an **older** `vite.config.ts` (hardcoded port 5179, hardcoded proxy, imports a `themeConfig` that doesn't exist there). Not matched by `pnpm-workspace.yaml`, so pnpm ignores it. Left in place — deleting it needs user approval. |
|
||
| DEAD-04 | Low | REF `henza_server/app/Console/Kernel.php` | `schedule()` is **empty** — no cron. Price refresh / expiry / birthday logic must be driven by OS cron or the `GET /` web route. |
|
||
| DEAD-05 | Low | REF `henza_server` | PayPing `returnUrl` points at `site/transaction/verify`, which is **not registered in any route file** — the payment callback is unreachable. |
|
||
|
||
## Known stubs in the target (not bugs, but not parity either)
|
||
|
||
| ID | Where | Description |
|
||
|---|---|---|
|
||
| STUB-01 | TGT both user apps | Registration persists nothing; favourites are in-memory only (lost on reload); notifications is a static empty state; home slides are 3 hardcoded placeholders in v2. |
|
||
| 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. |
|
||
|
||
## Found while classifying scope for D13
|
||
|
||
| ID | Severity | Where | Description | Evidence | Status |
|
||
|---|---|---|---|---|---|
|
||
| DOC-01 | Medium | DOC `docs/parity/_inv-ref-server.md:588` (§3.6) | The Phase 0 inventory undercounted `routes/admin.php`: it recorded **117** explicit routes where the file has **151** live ones (6 more are commented out). Every downstream total inherited the error, so the reference endpoint count was reported as **369** when it is **412** (admin 249, not 215). No code is affected — only the planning figures. | `grep -E "^\s*Route::(get\|post\|put\|patch\|delete\|any)" routes/admin.php \| grep -v "^\s*//" \| wc -l` → 151; resources 14×7=98; 151+98=249 | ✅ Corrected in `00-INVENTORY.md` §2.1 and `DECISIONS.md` §D13. `_inv-ref-server.md` left as the raw subagent record. |
|
||
| DOC-02 | Low | REF `routes/admin.php:38` | `GET admin/get-analytics-tak` reads `SingleCustomerOrders` (`Adm/AdminController.php:557-579`), so it is a **retail-storefront** endpoint despite its neutral name. A keyword scan for `wallet\|single\|comment` misses it. Noted so the D13 scope split stays correct — anything else named neutrally but querying `SingleCustomerOrders`, `Fav`, `Address`, `Comment` or `Transaction` is storefront too. | `AdminController.php:563` — `SingleCustomerOrders::select(...)` | ✅ Classified as ⏸️ storefront in D13 (the 31st admin storefront route). |
|
||
|
||
## Found during the SMS / MON-08 investigation (2026-09-22)
|
||
|
||
| ID | Severity | Where | Description | Evidence | Status |
|
||
|---|---|---|---|---|---|
|
||
| SEC-12 | **Critical** | REF `henza_server/app/Models/Sms.php:73-77,101-105` | Live SMS-panel production credentials (username, plaintext password, sender line) are **committed to git** in the reference, duplicated across two methods. An older provider's credentials sit commented out beside them. Anyone with repo access can send SMS on the owner's account at their cost. | Read the file. | Not ported — the target will read them from env only. **The owner should rotate these credentials**; they must be assumed compromised. |
|
||
| STUB-05 | Medium | REF `ApiMobileController.php:154-158,601-605`; `Adm/OrderController.php:54-55`; `Customer/OrderController.php:141-145` | Admin notification recipients are **hardcoded mobile numbers** scattered across controllers (six distinct numbers). Staff changes require a code deploy, and an ex-employee keeps receiving order data. | Read the files. | Port as `SMS_ADMIN_RECIPIENTS` (comma-separated, default empty) rather than copying the literals. Pending the SMS approval. |
|
||
| DEAD-06 | Low | REF `henza_pwa/src/views/cart.vue:181-182` | The cart sends `wage_weight: 5.7856000000000005` and `weight: 6.303744000000001` as **literal constants**, each with a `// how to get?!` comment. The server ignores both: it recomputes weight from `rash_weight`, and the `wage_weight` use is commented out (`ApiMobileController.php:549-551`). | Read both files. | Dead payload fields — do not port. |
|
||
|
||
## Found while applying the D0 / percent migrations (2026-09-22)
|
||
|
||
| ID | Severity | Where | Description | Evidence | Status |
|
||
|---|---|---|---|---|---|
|
||
| DEAD-07 | Low | TGT DB table `_goldbank_migrations` + `apps/api/src/prisma/migrate.ts` | **Re-assessed 2026-09-22 — not dead code.** It is the tracking table of the app's OWN migration runner (`src/prisma/migrate.ts:5`), which exists because the Docker deploy image is copy-only and ships no Prisma CLI, so `migrate deploy` cannot run there (`migrate.ts:7-13`). So the project has TWO ledgers on purpose: `_prisma_migrations` (CLI, used in development) and `_goldbank_migrations` (runtime runner, used in the container). `migrate diff` proposed `DROP TABLE` only because the table was not declared in the schema; the runner already refused to execute such a statement (`migrate.ts:190-196`). | Read `src/prisma/migrate.ts`. | ✅ Resolved: declared as an `@@ignore`d model, so drift is now **zero**. Table and all rows kept. |
|