138 lines
9.5 KiB
Markdown
138 lines
9.5 KiB
Markdown
# 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<View>("home")` and custom `popstate` listeners. Direct page loads to `/shop` or `/profile` revert to home page.
|
|
- *Solution*: Integrate `react-router-dom` with `<BrowserRouter>` 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**
|