# Phase 3.1 — Human Review Preparation and Master Backlog Critique - **Review Phase**: Phase 3.1 — Human Review Preparation and Master Backlog Critique - **Repository HEAD**: `715873b2ecc3a72ba974bb2a2be87c5ba82bd4e7` - **Execution Mode**: `REVIEW_ONLY_NO_CODE_MUTATION` - **Canonical Verified Findings Source**: `docs/audit/20-verified-findings-index.json` - **Original Task Count**: 10 (9 Implementation + 1 Verification) - **Final Task Count**: 10 (9 Implementation + 1 Verification) - **Tasks Modified / Refined**: 6 (`TASK-AUTH-001`, `TASK-FIN-001`, `TASK-FE-001`, `TASK-VERIFY-001`, `TASK-SEC-001`, `TASK-SEC-002`) - **Business Decisions Reclassified**: 3 (`DECISION-001`, `DECISION-002`, `DECISION-003`) --- ## 1. Executive Assessment A rigorous, engineering-grade review of the Phase 3 Master Task Backlog was conducted against the actual repository source code in `backend/` and `src/`. The review confirmed that all 14 verified findings are valid, but identified 5 critical technical refinements required before human approval: 1. **Authentication Token Storage Architecture (`TASK-AUTH-001`)**: Storing JWT access tokens in `localStorage` exposes the storefront to Cross-Site Scripting (XSS) token exfiltration. The recommended architecture has been revised to use in-memory token state in Zustand / Axios interceptors with optional HttpOnly cookie support for production environments. 2. **Financial Task Scope Cleanup (`TASK-FIN-001`)**: Inventory availability checking was removed from `TASK-FIN-001` as scope creep, because the Prisma `Product` schema (`schema.prisma`) does not contain an inventory/stock column. The task is strictly focused on arbitrary-precision `Decimal` arithmetic (`BE-001`) and batched `findMany` product lookups (`BE-002`). 3. **Business Decision Classification (`DECISION-002`)**: Payment gateway provider selection (`DECISION-002`) was removed from the canonical remediation backlog because no verified finding or active task requires payment gateway implementation. It is reclassified as `FUTURE_SCOPE`. 4. **Declarative Routing Justification (`TASK-FE-001`)**: Confirmed via `src/App.tsx` evidence that manual `useState("home")` view rendering causes direct link URL navigation failure and state desynchronization. React Router DOM (`react-router-dom`) is explicitly justified and detailed with an exact route table. 5. **Behavior-Oriented E2E Acceptance Criteria (`TASK-VERIFY-001`)**: Generic criteria ("100% of tests pass") have been replaced with precise, behavior-oriented verification assertions across Authentication, Security, Financial, Quality, and Frontend domains. --- ## 2. Findings That Require No Change The following 8 findings were verified to have clean, accurate root-cause mappings and task assignments requiring no structural change: - **`SEC-003`** -> Plaintext OTP disclosure in API payload (Handled in `TASK-SEC-002`). - **`ADM-001`** -> Missing RBAC guard on settings API (Handled in `TASK-SEC-003`). - **`BE-001`** -> Floating-point monetary calculation error (Handled in `TASK-FIN-001`). - **`BE-002`** -> Synchronous N+1 query lookup in order processing (Handled in `TASK-FIN-001`). - **`TS-002`** -> Missing `slug` field in Prisma seed script (Handled in `TASK-BUILD-001`). - **`TS-003`** -> Non-type-only `Response` import under `isolatedModules` (Handled in `TASK-BUILD-001`). - **`TEST-001`** -> Stale spec assertions for controller return types (Handled in `TASK-BUILD-001`). - **`DOC-001`** -> Static Swagger specification desynchronization (Handled in `TASK-DOC-001`). --- ## 3. Findings Requiring Task Refinements - **`ARCH-001`**: Refined token storage mechanism in `TASK-AUTH-001`. Instead of suggesting `localStorage` as a primary storage target, the task explicitly warns against `localStorage` due to XSS vulnerability and specifies in-memory token state + HttpOnly cookie evaluation. - **`SEC-001`**: Refined threat model and startup failure behavior in `TASK-SEC-001` to enforce mandatory length checks (minimum 32 bytes) and immediate process exit (`process.exit(1)`) on missing secrets. - **`SEC-002`**: Expanded `TASK-SEC-002` to explicitly specify rate-limiting and expiration boundaries for SMS OTP codes. - **`FE-001` & `TS-001`**: Explicitly mapped storefront route parameters and sub-view interface types in `TASK-FE-001`. --- ## 4. Tasks Requiring Splitting - **Evaluation**: No tasks required physical splitting. `TASK-FIN-001` was originally considered for splitting, but since inventory checking was identified as unevidenced scope creep and removed, keeping `Decimal` calculation (`BE-001`) and query batching (`BE-002`) together in `TASK-FIN-001` is optimal because both operate inside `OrdersService.create` in `backend/src/orders/orders.service.ts`. --- ## 5. Tasks Requiring Merging - **Evaluation**: The current 9 implementation tasks represent the minimal, non-overlapping task set. No further merging is required. --- ## 6. Unsupported Assumptions Identified 1. **Assumption in `TASK-AUTH-001`**: Assumed `localStorage` was the standard production auth token storage mechanism. - *Correction*: `localStorage` is vulnerable to XSS token theft. Revised to specify in-memory Zustand token state with Bearer header interceptor in `src/services/api.ts`, with architectural guidance for HttpOnly cookies. 2. **Assumption in `TASK-FIN-001`**: Assumed `OrdersService.create` needed inventory stock deduction checks. - *Correction*: Inspection of `backend/prisma/schema.prisma` shows `Product` has no `stock` or `inventory` field. Inventory deduction claims removed to prevent scope creep. --- ## 7. Scope Creep Identified and Removed - **Removed Item**: Inventory availability checking and stock deduction in `TASK-FIN-001`. - **Removed Item**: Payment Gateway Provider Selection (`DECISION-002`) from active remediation backlog. Moved to `FUTURE_SCOPE`. --- ## 8. Security Deep-Dive Review - **`TASK-SEC-001` (JWT Secret Enforcement)**: - *Threat Model*: Attackers forging JWT signatures with role `Admin` or arbitrary `sub` (userId) when `JWT_SECRET` falls back to `'super-secret-key-canina'`. - *Failure Behavior*: Server startup terminates immediately (`process.exit(1)`) with `FATAL: JWT_SECRET environment variable is missing or insecure`. - **`TASK-SEC-002` (CSPRNG OTP & Payload Sanitization)**: - *Threat Model*: PRNG state prediction of `Math.random()` to guess 5-digit OTPs; exploitation of plaintext `code` returned in API JSON response. - *Remediation*: Use `crypto.randomInt(10000, 100000)`. Strip `code` key from response. Enforce 2-minute OTP expiry in Redis/memory. - **`TASK-SEC-003` (Admin Settings RBAC Enforcement)**: - *Threat Model*: Authenticated `User_PetOwner` clients sending requests to `/api/settings` to overwrite site branding or scientific terms. - *Remediation*: Apply `@UseGuards(JwtAuthGuard, RolesGuard)` and `@Roles('Admin')` on `SettingsController`. --- ## 9. Architecture Review - **Frontend Routing (`TASK-FE-001`)**: - `src/App.tsx` evidence shows manual `useState("home")` and custom `popstate` listeners. Direct page loads to `/shop` or `/profile` revert to home page. - *Solution*: Integrate `react-router-dom` with `` and explicit route map: `/`, `/shop`, `/product/:id`, `/wiki`, `/profile/*`, `/checkout`, `/order-success`, `/order-tracking`. - **Backend API Client Integration (`TASK-AUTH-001`)**: - Storefront `src/services/authService.ts` currently has partial `sendOtp` and `verifyOtp` methods. Wiring `userStore` to update in-memory state on successful `verifyOtp` completes the protocol alignment. --- ## 10. Dependency & Wave Adjustments The execution waves have been refined to prioritize unblocking CI early: - **Wave 0 — Security & Financial Core (P0)**: `TASK-SEC-001`, `TASK-SEC-002`, `TASK-SEC-003`, `TASK-FIN-001`. - **Wave 1 — Build Health & Spec Alignment (P1)**: `TASK-BUILD-001` (*Unblocks CI Pipeline!*). - **Wave 2 — Auth Protocol & Storefront Integration (P1)**: `TASK-AUTH-001` (*Depends on `TASK-SEC-002`*). - **Wave 3 — Frontend Routing & CI Pipeline (P2)**: `TASK-FE-001`, `TASK-DEVOPS-001` (*`TASK-DEVOPS-001` depends on `TASK-BUILD-001`*). - **Wave 4 — Documentation & Behavior Verification (P3 / Verification)**: `TASK-DOC-001`, `TASK-VERIFY-001`. --- ## 11. Acceptance Criteria Refinements All generic acceptance criteria were replaced with concrete, observable, testable assertions. Example (`TASK-FIN-001`): - *Old*: "Order total is accurate." - *New*: "`OrdersService.create` accumulates subtotals using `Decimal.add()` without primitive JS number conversion; unit spec asserts `19.99 * 3 + 5.01 = 65.00` with 0 decimal rounding artifacts; `this.prisma.product.findMany` is called exactly once per checkout request." --- ## 12. Business and Product Decision Classification | Decision ID | Title | Category | Description | Status | |---|---|---|---|---| | **DECISION-001** | SMS Gateway Provider Selection | `PRODUCT_DECISION` | Production SMS Gateway selection (e.g. Kavenegar, FarazSMS, Twilio) for NestJS `AuthService.sendOtp`. Dev mode uses stdout logger. | `PENDING_PRODUCT_INPUT` | | **DECISION-002** | Live Payment Gateway Provider Selection | `FUTURE_SCOPE` | Merchant payment gateway provider selection for wallet deposits. Unrelated to current audit findings. | `FUTURE_SCOPE` | | **DECISION-003** | Storefront vs Admin Architecture | `PRODUCT_DECISION` | Decision whether Admin UI remains embedded in SPA (`/admin/*`) or isolated into a standalone admin portal repo. | `PENDING_PRODUCT_INPUT` | --- ## 13. Final Recommended Backlog Summary - **Total Tasks**: 10 (9 Implementation + 1 Verification) - **Status**: **100% READY FOR HUMAN APPROVAL**