fix(auth): tell the user when the number is not registered (D28)
Reported by the owner: a number that is not in the system produced no error at all. The app advanced to the code box and then nothing happened, and there was no way to tell apart "the SMS is late", "the gateway is down" and "this number was never registered" — the only one of the three the person can actually do something about. Step 1 now says which it is: - unregistered number -> 404 «این شماره در سیستم ثبت نشده است…» - SMS send failed -> 503 «ارسال پیام کوتاه … ممکن نیست…» - sales expert, account unusable -> 403, its own message and both apps surface the server's Persian message for a 404 exactly as they already did for a 429. Without that second half the person would still have seen the generic «خطا در ارسال کد», which is the confusion being removed. This restores the reference's behaviour (firstOrFail() in ApiMobileController::userAuth:43 returns 404); the opaque answer was our deviation, not this. The anti-enumeration protection it was there for is deliberately reduced at step 1 on the owner's instruction — the cost was being paid by real users and by support. Still standing: the per-mobile and per-IP send throttles, which the reference never had, so enumeration is rate-bounded rather than free, and verifyOtp keeps one message for every failure reason. ISSUES.md SEC-03's "no enumeration" claim is corrected rather than left to go stale. Two more instances of the same shape turned up while fixing it: A failed SMS send also returned success, because SmsService never throws (D20), so a dead gateway produced the identical dead end. D20 exists so a failed SMS cannot break an operation — an order must still be placed — but in a login the SMS IS the operation, so it now fails loudly. The local-dev path keys off SmsService's own `sms-not-configured` result rather than re-testing the config: with D26's default relay driver, smsConfigured() is true on any machine with an external IPv4, so a config test would have called a dev box configured and then failed its login. The new test caught precisely that on its first run. Three states return no trading account for a sales expert who does exist. Under the new 404 each would have claimed the number was not registered — false, and it would send support hunting a registration problem that is not there. Also corrects both apps' LoginView header comment, which still described this flow as stubbed: no SMS provider, any 4-digit code accepted, step 1 matching the customer list in the client (that was the login leak, D8). All three had been untrue for weeks and the stale text misled a reader during this session. requestOtp had NO test coverage: the 28-test customer-auth suite stayed green through a change to its response for every unregistered number, so its greenness was not treated as evidence. request-otp.spec.ts adds 9 cases, including that the dev fallback does not apply in production. api: 295/295 tests, 27 suites, typecheck clean; both apps vue-tsc clean. اپ: اگر شماره شما در سیستم ثبت نشده باشد، همان را میگوید و دیگر بیدلیل به صفحه کد نمیرود. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
89e2d7937e
commit
c518f9995b
+21
-1
@@ -17,7 +17,7 @@ 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~~ **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-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. Verified end-to-end by `auth-smoke.sh`. ⚠️ **"no enumeration" no longer holds at step 1** — owner ruling 2026-10-12 (see AUTH-01 below) makes `requestOtp` answer an unregistered number with 404, because the opaque answer was unusable in support. Step 2 (`verifyOtp`) is still single-message for every failure reason, and the per-IP send throttle still bounds any enumeration attempt. |
|
||||
| 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`. |
|
||||
@@ -223,3 +223,23 @@ apps, as do the `sts_verify_step_two` guard and v2's cart stock ceiling
|
||||
| **APP-03** | Medium (UX) | REF `henza_pwa/src/router/index.ts:253-261` vs TGT `apps/user-app/src/router/index.ts`, `user-app-v2` same | A guest tapping a personal page (orders, transactions, profile, favourites, complete-profile) was redirected **in silence**, so they landed on the login box with nothing saying why. Legacy showed «ابتدا وارد شوید». | Read both guards; legacy calls `showNotify('ابتدا وارد شوید')` then `next({name:'home'})`. | ✅ **FIXED 2026-10-12** — `toast.info('ابتدا وارد شوید')` in both apps' `beforeEach`. **«عمداً تکرار نشد — D3/الف»** for the destination: legacy sent the guest to `home` and lost where they were going; the owner chose to keep the target's `query.redirect`, so signing in returns them to the page they asked for. Also applied via `meta.requiresAuth` rather than legacy's hand-kept list of five names, which additionally covers the two sales-expert pages. Only reached when the IAM is off (the default) or unreachable — silent SSO leaves the page before this runs, and a toast on a document being replaced could not be read. |
|
||||
| **APP-04** | Low | REF `henza_pwa/src/router/index.ts:263-266` vs TGT both apps `stores/configStore.ts` | Legacy's `isLoaded` router guard sent a signed-in user to `home` from any route without `meta.skipLoadedCheck` until its one big bootstrap request had landed. | Read both; the target's store header already said `isLoaded` was not ported. | ⏹️ **«عمداً تکرار نشد — D3/ب»** (owner, 2026-10-12). The target has no bootstrap request — every page fetches its own data — so the flag would guard nothing and only add a bounce: opening a shared product link would land on home instead of the product. Porting it would port the breakage, not the behaviour (D4). Recorded in both apps' `configStore.ts` so a later reader finds the reasoning where they would look for the flag. |
|
||||
| **ANLY-01** | Low | REF `henza_server/app/Http/Controllers/Api/LogController.php`, `henza_pwa/src/router/customAnalytics.ts`; routes `api.php:57-58` | Per-page dwell-time analytics: `POST /log/create/{title}` opened a row (`start_at`, `ip`, `title`, `customer_id`, optional `product_id`), `GET /log/update/{id}` closed it; four admin report endpoints aggregated it. Entirely absent from the target — no table, no routes, no calls. | Read the controller, the composable and the routes. | ⏸️ **DEFERRED — lowest priority, may never be built** (owner, 2026-10-12: «ج اصلا اولیوت نداره جزو اخرین چیز هایی هست که شاید درستش کنیم»). The full design is parked in `docs/PHASE2-D3-PLAN.md` §"شکاف ۳", including two reference defects **not** to copy if it is ever built: `log/create` required no auth and accepted any title (anyone could fill the table from outside), and `log/update` was a `GET` that wrote. Open owner questions there too: retention (suggested 90 days — legacy never deleted) and whether to store the IP at all (suggested no). |
|
||||
|
||||
## App login — owner ruling 2026-10-12 (says what went wrong)
|
||||
|
||||
Reported by the owner: «اگر کاربری در اپ نبود خطا نمیدادی کاربر یافت نشد
|
||||
میبردیش توی صفحه ارسال کد و واقعا نمیشد فهمید خطا از پروژه هست یا نه پیامک نمیاد
|
||||
یا نه واقعا نیست».
|
||||
|
||||
| ID | Severity | Where | Description | Evidence | Status |
|
||||
|---|---|---|---|---|---|
|
||||
| **AUTH-01** | **High (support/UX)** | TGT `apps/api/src/modules/customer-auth/customer-auth.service.ts` `requestOtp`, both apps' `LoginView.vue` | Step 1 returned `{ sent: true }` for a number no customer holds, so the app advanced to the code box and the person waited for an SMS that was never sent. Three very different causes were indistinguishable on screen: the SMS is late, the gateway is down, or the number was never registered — the only one the person can act on. | Read the guard (the opaque `return { sent: true }` with the enumeration note) and the apps' catch, which showed a generic «خطا در ارسال کد» for anything that was not a 429. | ✅ **FIXED.** 404 + «این شماره در سیستم ثبت نشده است…»; both apps now surface the server's Persian message for 404 as they already did for 429. **This restores the reference's behaviour** (`firstOrFail()`, `ApiMobileController::userAuth:43` → 404) — the opaque response was the deviation, not this. Enumeration stays bounded by `assertSendAllowed` (per-mobile and per-IP throttles), which the reference did not have, and step 2 is still a single message for every failure reason. |
|
||||
| **AUTH-02** | **High (support/UX)** | TGT same file, `deliver` | A failed SMS send **also** returned success. `SmsService` never throws — a dead gateway is a log line (D20) — so an outage produced the identical "waiting at the code box" dead end, with nothing visible server-side either unless someone read the logs. | `SmsService.deliver` returns `{ ok: false, … }` and `requestOtp` ignored the result. | ✅ **FIXED.** A failed send is now 503 + «ارسال پیام کوتاه در حال حاضر ممکن نیست…». D20 protects an *operation* from a dead gateway (an order must still be placed); in a login the SMS **is** the operation, so there is nothing to protect by staying quiet. The code row is left in place (harmless, expires on its own). The local-dev path is preserved: when `SmsService` reports its own `sms-not-configured` outside production the code is logged and the login completes. |
|
||||
| **AUTH-03** | Medium (wrong message) | TGT same file, `resolveExpertTradingAccount` | Found while fixing AUTH-01. Three states return no trading account for a sales expert who **does** exist (linked customer missing/deactivated/archived, or no `SalesExpertProfile`). Previously all three fell into the opaque success; with AUTH-01 they would have reported «این شماره ثبت نشده است» — a false statement that would send support hunting a registration problem that is not there. | Read the method: two `return null` paths plus the archived/inactive one. | ✅ **FIXED** in the same pass: 403 «حساب شما غیرفعال است…» and «حساب شما کامل نیست…», each distinct from the unknown-number message. The missing-profile case is now logged at `error`, not `warn` — it means something bypassed both paths that create a profile (D17), i.e. a server-side defect. |
|
||||
| **DOC-01** | Low (misleading) | TGT both apps `src/views/user/LoginView.vue` header comment | The file header still said the OTP flow was **stubbed** — "the SMS-send step is stubbed — no SMS provider exists", "Step 2 (OTP code) is NOT verified — any 4-digit code is accepted" — and that step 1 matched against `GET /api/customers` **in the client**, which was the login leak (D8). All three had been untrue for weeks. | Read the comment against the code beside it. | ✅ **FIXED 2026-10-12** — corrected in both apps rather than deleted, with a note that the stale text actually misled a reader during this session. |
|
||||
|
||||
**Test gap this exposed.** `requestOtp` had **no test coverage at all**: the whole
|
||||
28-test customer-auth suite stayed green through a change that altered its
|
||||
response for every unregistered number. `request-otp.spec.ts` now covers 9 cases
|
||||
— the happy path, the unknown number, Persian-digit input, a refused gateway, the
|
||||
dev no-gateway fallback, that the fallback does **not** apply in production, and
|
||||
the two sales-expert states.
|
||||
|
||||
Reference in New Issue
Block a user