diff --git a/CODE_REVIEW_REPORT.md b/CODE_REVIEW_REPORT.md new file mode 100644 index 00000000..5acb754a --- /dev/null +++ b/CODE_REVIEW_REPORT.md @@ -0,0 +1,491 @@ +# Comprehensive Code Review Report + +**Repository:** StormCom - Multi-Tenant SaaS E-Commerce Platform +**Date:** 2026-04-01 +**Scope:** 291 API route files, 110 page routes +**Branch:** `cursor/comprehensive-repository-project-review-8eec` + +--- + +## Table of Contents + +1. [Architecture Overview](#1-architecture-overview) +2. [API Route Audit](#2-api-route-audit) +3. [Page Route Audit](#3-page-route-audit) +4. [Security Findings](#4-security-findings) +5. [Issues Summary](#5-issues-summary) + +--- + +## 1. Architecture Overview + +### Auth Infrastructure + +| Component | Implementation | +|---|---| +| **Auth Provider** | NextAuth.js with JWT strategy | +| **Providers** | Email (magic link via Resend) + Credentials (bcrypt) | +| **Session Storage** | JWT tokens with cached permissions | +| **RBAC** | Permission-based (`products:read`, `orders:create`, etc.) with role hierarchy | +| **Roles** | `SUPER_ADMIN`, `OWNER`, `ADMIN`, `STORE_ADMIN`, `SALES_MANAGER`, `INVENTORY_MANAGER`, `CUSTOMER_SUPPORT`, `CONTENT_MANAGER`, `MARKETING_MANAGER`, `DELIVERY_BOY` | +| **Multi-tenancy** | Store-scoped access via `storeId` verification | +| **API Tokens** | PBKDF2-hashed scoped tokens (`stc_live_*` / `stc_test_*`) | + +### Middleware Patterns + +Three auth patterns used across routes: + +1. **`apiHandler(options, handler)`** - Preferred. Combines auth + CSRF + permission + error handling. Used by ~60% of routes. +2. **Manual `getServerSession()` + `checkPermission()`** - Legacy pattern. Used by ~30% of routes. +3. **No auth (public endpoints)** - Used by ~10% of routes (storefront, search, webhooks). + +### Response Format + +Two competing response formats exist: + +- **`api-middleware.ts`**: `{ error: string }` or raw data +- **`api-response.ts`**: `{ success: boolean, data: T, error?: string, code?: string }` + +--- + +## 2. API Route Audit + +### Products Domain + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/products` | GET, POST | `apiHandler` + `products:read/create` | Zod (POST), query params (GET) | GET: `requireStore` middleware; POST: `resolveTenantContext` | None critical | +| `/api/products/[id]` | GET, PATCH, DELETE, PUT | `apiHandler` + `products:read/update/delete` | Zod (params via `cuidSchema`) | `verifyStoreAccess(storeId)` | **ISSUE: Debug `console.log` statements in PATCH handler leak request details in production** | +| `/api/products/upload` | POST, PUT | `apiHandler` + `products:create` | File type, size, storeId format regex | `verifyStoreAccess` + `isValidStoreId` path traversal check | SVG excluded (prevents stored XSS) - good | +| `/api/products/export` | GET | Manual session + `getAuthorizedStoreId` | None (query params not validated) | `getAuthorizedStoreId` | **ISSUE: Not using `apiHandler`, no CSRF protection, inconsistent response format** | +| `/api/products/bulk` | POST | Manual session + `getAuthorizedStoreId` | Zod schema (1-100 products) | `getAuthorizedStoreId` | **ISSUE: Not using `apiHandler`, no permission check (should require `products:create`)** | +| `/api/products/import` | POST | `apiHandler` + `products:create` | Zod + CSV parsing via papaparse | `verifyStoreAccess` | Good | +| `/api/products/[id]/store` | GET | `apiHandler` + `products:read` | Zod `cuidSchema` | Permission only, no store-level check | **ISSUE: Returns storeId for any product the user has product read access to - no store-scoped filtering** | +| `/api/products/[id]/reviews` | GET | `apiHandler` with empty options `{}` | Zod query schema | None | Public endpoint - intentional | + +### Orders Domain + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/orders` | GET, POST | Manual `checkPermission` + `getServerSession` | Zod (query + body) | `hasStoreAccess` | **ISSUE: GET checks permission before session - redundant session check after. POST reads storeId from `x-store-id` header - no validation that header value is a CUID** | +| `/api/orders/[id]` | GET, PATCH, DELETE | Manual session + permission | Minimal | `hasStoreAccess` | **CRITICAL: GET allows unauthenticated access with email/phone verification. An attacker who knows an order ID and customer email can access full order details including payment info** | +| `/api/orders/[id]/status` | PATCH | `apiHandler` + `orders:update` | Zod schema | `hasStoreAccess` | Good | +| `/api/orders/[id]/cancel` | POST | `apiHandler` + `orders:update` | Zod schema | `hasStoreAccess` | Good | +| `/api/orders/[id]/refund` | POST | `apiHandler` + `orders:update` | Zod schema | `hasStoreAccess` | Good - double-checks session for userId | +| `/api/orders/[id]/fulfillments` | Various | `apiHandler` + `orders:update` | Zod | `hasStoreAccess` | Good | +| `/api/orders/[id]/invoice` | GET | `apiHandler` + `orders:read` | Zod | `hasStoreAccess` | Good | +| `/api/orders/bulk` | PATCH | Manual session + `getAuthorizedStoreId` | Zod (1-100 order IDs) | `getAuthorizedStoreId` | **ISSUE: Not using `apiHandler`, no CSRF protection. No explicit permission check for `orders:manage`** | +| `/api/orders/export` | GET | Manual session + `getAuthorizedStoreId` | None | `getAuthorizedStoreId` | **ISSUE: Same as products/export - no `apiHandler`, no permission check** | +| `/api/orders/stream` | GET (SSE) | Manual session | None | Custom `checkStoreAccess` with in-memory cache | **ISSUE: Store access cache (`storeAccessCache`) is a memory leak - Map grows unbounded. Uses raw SQL which could be vulnerable if inputs change** | +| `/api/orders/track` | GET | Public/partial | Varies | Varies | Public tracking endpoint | +| `/api/orders/cod/verify` | POST | Various | Various | Various | COD verification | +| `/api/orders/check-updates` | GET | Session | Query params | Store access | Polling endpoint | + +### Store Management (Admin - `/api/stores/[id]/*`) + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/stores/[id]` | GET, PUT, PATCH, DELETE | `apiHandler` + `stores:read/update/delete` | Zod `cuidSchema` | `requireStoreAccessCheck` | **NOTE: PATCH is aliased to PUT - no partial update semantics** | +| `/api/stores/[id]/staff` | GET, POST | `apiHandler` + `staff:read/create` | Zod (POST: email, role) | Custom `checkStoreAccess` | Good - verifies owner/admin before staff invite | +| `/api/stores/[id]/staff/[staffId]` | PATCH, DELETE | `apiHandler` + `staff:update/delete` | Zod | Store access | Good | +| `/api/stores/[id]/staff/accept-invite` | POST | `apiHandler` | None | Session-based | Good | +| `/api/stores/[id]/settings` | GET, PATCH | `apiHandler` + `stores:read/update` | Zod | Permission only | **ISSUE: Returns mock data - not connected to database** | +| `/api/stores/[id]/stats` | GET | `apiHandler` + `stores:read` | Zod | Permission | Good | +| `/api/stores/[id]/theme` | GET, PATCH | `apiHandler` + `stores:read/update` | Zod | Permission | Good | +| `/api/stores/[id]/domain` | GET, POST | `apiHandler` + `stores:update` | Zod | Permission | Good | +| `/api/stores/[id]/domain/verify` | POST | `apiHandler` + `stores:update` | Zod | Permission | Good | +| `/api/stores/[id]/pwa` | GET, PATCH | `apiHandler` + `stores:read/update` | Zod | Permission | Good | +| `/api/stores/[id]/storefront` | GET, PUT | `apiHandler` + `stores:read/update` | Zod | Permission | Good | +| `/api/stores/[id]/storefront/draft` | GET, PUT | `apiHandler` + `stores:update` | Zod | Permission | Good | +| `/api/stores/[id]/storefront/publish` | POST | `apiHandler` + `stores:update` | None | Permission | Good | +| `/api/stores/[id]/storefront/versions` | GET | `apiHandler` + `stores:read` | None | Permission | Good | +| `/api/stores/[id]/role-requests` | GET, POST | `apiHandler` | Zod | Permission | Good | +| `/api/stores/[id]/custom-roles` | GET, POST | `apiHandler` + `staff:create` | Zod | Permission | Good | +| `/api/stores/[id]/manifest` | GET | Public | None | None | PWA manifest - public by design | +| `/api/stores/[id]/sw` | GET | Public | None | None | Service worker - public by design | +| `/api/stores/[id]/pathao/settings` | GET, PATCH | `apiHandler` | Zod | Permission | Good | + +### Public Storefront (`/api/store/[slug]/*`) + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/store/[slug]` | GET | **None** | Slug param only | None | Public - intentional. **ISSUE: Exposes `organizationId` in response - potential info leak** | +| `/api/store/[slug]/orders` | POST | **None** (guest checkout) | Zod (comprehensive) | None (store validated by slug) | **Rate limited (10/min)**, idempotency key support, server-side pricing. Well implemented | +| `/api/store/[slug]/orders/[orderId]` | GET | **None** | None | Store validated by slug | **CRITICAL: No authentication. Anyone who knows an orderId can view full order details, items, payment status, and shipping address** | +| `/api/store/[slug]/orders/[orderId]/invoice` | GET | **None** | None | Store validated by slug | **CRITICAL: No authentication. Anyone who knows an orderId can download the invoice PDF** | +| `/api/store/[slug]/orders/[orderId]/verify-payment` | POST, GET | **None** | None (orderId from path) | Store validated by slug | **CRITICAL: Unauthenticated payment verification/mutation. An attacker could trigger payment status polling for any order** | +| `/api/store/[slug]/orders/track` | GET | **None** | Query params | None | Public tracking - intentional | +| `/api/store/[slug]/coupons/validate` | POST | **None** | Zod | None | Public - intentional | +| `/api/store/[slug]/cart/validate` | POST | **None** | Zod | None | Public - intentional | +| `/api/store/[slug]/payment-methods` | GET | **None** | None | None | Public - intentional | + +### Checkout Domain + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/checkout/validate` | POST | `apiHandler` + `checkout:validate` | Zod | None | **ISSUE: Requires `checkout:validate` permission - but guest checkout should be public?** | +| `/api/checkout/complete` | POST | `apiHandler` + `checkout:complete` | Zod | None | **ISSUE: Same concern - requires auth permission but checkout is supposed to support guests** | +| `/api/checkout/shipping` | POST | `apiHandler` | Zod | None | Good | +| `/api/checkout/payment-intent` | POST | `apiHandler` | Zod | None | Good | + +### Payment Domain + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/payments/configurations` | GET, POST | Manual session | POST: basic field check only | Org membership | **ISSUE: No Zod validation on POST. No permission check. Any org member can modify payment gateway config** | +| `/api/payments/configurations/toggle` | PATCH | Session | Minimal | Org membership | **ISSUE: Same as above** | +| `/api/payments/sslcommerz/initiate` | POST | Manual session | Basic field check | None | **ISSUE: No store access check. No Zod validation. Trusts client-provided `amount` and `organizationId`** | +| `/api/payments/bkash/callback` | GET | **None** (payment callback) | `paymentID` from query | Implicit via payment attempt lookup | Good - validates amount/currency mismatch | +| `/api/payments/nagad/callback` | GET | **None** (payment callback) | Query params | Implicit | Similar to bKash | +| `/api/payments/transactions` | GET | Session | Query params | Org membership | Good | + +### Inventory Domain + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/inventory` | GET, POST | `apiHandler` + `inventory:read/manage` | Zod | GET: `requireStore`; POST: `getAuthorizedStoreId` | Good | +| `/api/inventory/adjust` | POST | `apiHandler` + `inventory:update` | Zod | `getAuthorizedStoreId` | Good | +| `/api/inventory/bulk` | PATCH | Session + `getAuthorizedStoreId` | Zod | `getAuthorizedStoreId` | **ISSUE: Not using `apiHandler`** | +| `/api/inventory/export` | GET | Session + `getAuthorizedStoreId` | None | `getAuthorizedStoreId` | **ISSUE: Not using `apiHandler`** | +| `/api/inventory/history` | GET | Session + `getAuthorizedStoreId` | Query params | `getAuthorizedStoreId` | Good | +| `/api/inventory/low-stock` | GET | Session + `getAuthorizedStoreId` | Query params | `getAuthorizedStoreId` | Good | + +### Notifications Domain + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/notifications` | GET | `apiHandler` (auth required) | Zod | User-scoped | Good - supports cursor + offset pagination | +| `/api/notifications/[id]` | PATCH, DELETE | `apiHandler` | Zod | User-scoped | Good | +| `/api/notifications/[id]/read` | PATCH | `apiHandler` | None | User-scoped | Good | +| `/api/notifications/read` | PATCH | `apiHandler` | None | User-scoped | Bulk mark read | +| `/api/notifications/mark-all-read` | PATCH | `apiHandler` | None | User-scoped | Good | +| `/api/sse/notifications` | GET (SSE) | Session | None | User-scoped | Good | + +### Chat / AI Domain + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/chat/generate` | POST | Manual session | Custom validation + sanitization | None | Good - input sanitization strips HTML tags | +| `/api/chat/ollama` | POST | Session | Various | None | Ollama proxy | +| `/api/chat/sessions` | GET, POST | Session | Zod | None | Good | +| `/api/chat/sessions/[sessionId]` | GET, PATCH, DELETE | Session | Zod | None | **ISSUE: No ownership check - user A could access user B's chat session** | +| `/api/chat/models` | GET | Session | None | None | Good | +| `/api/chat/openai/v1/*` | Various | Session/Bearer token | Various | None | OpenAI-compatible proxy | +| `/api/chat/websearch` | POST | Session | Zod | None | Good | +| `/api/chat/image-generate` | POST | Session | Zod | None | Good | +| `/api/chat/semantic-search/products` | POST | Session | Zod | None | Good | +| `/api/chat/history` | GET | Session | Query params | None | Good | +| `/api/chat/usage` | GET | Session | None | None | Good | +| `/api/chat/webfetch` | POST | Session | URL validation | None | **ISSUE: SSRF risk - fetches arbitrary URLs server-side** | + +### Organization Domain + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/organizations` | GET, POST | `apiHandler` + `organizations:read/create` | Zod | Membership-scoped | Good - has rate limiting on POST | +| `/api/organizations/[slug]/invite` | POST | `apiHandler` | Zod | Membership-scoped | Good | + +### Search Domain + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/search` | GET | **Optional** (public for storefront) | Zod | Public stores accessible; authenticated users need `getAuthorizedStoreId` | Good - dual auth path | +| `/api/search/suggest` | GET | **None** | Query params | Store validated | Public autocomplete | +| `/api/search/analytics` | POST | Session | Zod | Store access | Good | + +### Media Domain + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/media/upload` | POST, OPTIONS | Manual session | MIME type + magic bytes + extension whitelist + size | `canManageStoreMedia` | Excellent - magic byte validation prevents MIME spoofing | +| `/api/media/list` | GET | Session | Query params | Store access | Good | + +### Integrations Domain + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/integrations` | GET | `apiHandler` | None | User-scoped | Good | +| `/api/integrations/[id]` | GET, PATCH, DELETE | `apiHandler` | Zod | User-scoped | Good | +| `/api/integrations/sslcommerz` | GET, POST | Session | Basic | Org-scoped | **ISSUE: No Zod validation on POST** | +| `/api/integrations/sslcommerz/test` | POST | Session | None | Org-scoped | Test endpoint | +| `/api/integrations/facebook/*` | Various | Session | Various | Various | Facebook integration endpoints | + +### Miscellaneous Routes + +| Route | Methods | Auth | Validation | Store Access | Issues | +|---|---|---|---|---|---| +| `/api/openapi` | GET | **None** | None | None | Public - intentional | +| `/api/metrics` | GET | Bearer token OR super admin session | None | None | Good - dual auth (token + session) | +| `/api/demo/create-store` | POST | Session | None | None | **ISSUE: Only env check prevents production use. No Zod validation. No rate limiting** | +| `/api/permissions` | GET | `apiHandler` | None | None | Returns role-permission mappings | +| `/api/store-staff` | GET, POST | `apiHandler` | Zod | Various | Good | +| `/api/store-requests` | GET, POST | `apiHandler` | Zod | Various | Good | +| `/api/product-attributes` | GET, POST | `apiHandler` | Zod | Store access | Good | +| `/api/reviews` | GET | `apiHandler` | Zod | Store access | Good | +| `/api/reviews/[id]` | GET, PATCH, DELETE | `apiHandler` | Zod | Store access | Good | +| `/api/reviews/[id]/approve` | PATCH | `apiHandler` | Zod | Store access | Good | +| `/api/coupons` | GET, POST | `apiHandler` + `coupons:read/create` | Zod | Store access verified | Good | +| `/api/coupons/[id]` | GET, PATCH, DELETE | `apiHandler` | Zod | Store access | Good | +| `/api/coupons/validate` | POST | `apiHandler` | Zod | Store access | Good | +| `/api/landing-pages` | GET, POST | `apiHandler` | Zod | Store access | Good | +| `/api/landing-pages/[id]` | GET, PATCH, DELETE | `apiHandler` | Zod | Store access | Good | +| `/api/landing-pages/[id]/publish` | POST | `apiHandler` | None | Store access | Good | +| `/api/landing-pages/[id]/duplicate` | POST | `apiHandler` | None | Store access | Good | +| `/api/landing-pages/[id]/track` | POST | **None** | None | None | Public page view tracking | +| `/api/landing-pages/templates` | GET | `apiHandler` | None | None | Good | +| `/api/settings/ai` | GET, PATCH | `apiHandler` | Zod | User-scoped | Good | +| `/api/settings/ai/test` | POST | `apiHandler` | Zod | User-scoped | Good | +| `/api/shipping/pathao/*` | Various | Session | Various | Org/store-scoped | Good | +| `/api/shipping/rates` | POST | `apiHandler` | Zod | Store access | Good | + +--- + +## 3. Page Route Audit + +### Auth Pages + +| Route | Rendering | Auth Protection | Data Fetching | Error Boundary | +|---|---|---|---|---| +| `/login` | **Client** (`"use client"`) | None (login page) | None | None | +| `/signup` | **Client** | None (signup page) | None | None | +| `/forgot-password` | **Client** | None | None | None | +| `/verify-email` | **Client** | None | None | None | +| `/pending-approval` | **Client** | None | None | None | + +### Admin Pages (`/admin/*`) + +| Route | Rendering | Auth Protection | Data Fetching | Error Boundary | +|---|---|---|---|---| +| `/admin` (layout) | **Server** | `getServerSession` + DB check `isSuperAdmin` -> redirect | Prisma direct | None | +| `/admin` (page) | **Server** | Layout protection | Prisma direct (stats, users, activity) | Suspense boundaries | +| `/admin/users` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/users/[id]` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/users/pending` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/stores` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/stores/[id]` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/stores/create` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/stores/requests` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/settings` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/setup-payment` | **Server** | Layout protection | Prisma direct | None | +| `/admin/activity` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/metrics` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/analytics` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/organizations` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/roles/requests` | **Server** | Layout protection | Prisma direct | Suspense | +| `/admin/notifications` | **Server** | Layout protection | Prisma direct | Suspense | + +### Dashboard Pages (`/dashboard/*`) + +| Route | Rendering | Auth Protection | Data Fetching | Error Boundary | +|---|---|---|---|---| +| `/dashboard` | **Server** | `getServerSession` -> redirect | Client component delegated | `` | +| `/dashboard/stores` | **Server/Client hybrid** | Session check | API calls from client | Various | +| `/dashboard/stores/[storeId]/*` | **Server/Client hybrid** | Session check | API calls + Prisma | Various | +| `/dashboard/products` | **Client** | Session check via API | API calls | None | +| `/dashboard/products/[id]` | **Client** | Session check via API | API calls | None | +| `/dashboard/products/new` | **Client** | Session check via API | API calls | None | +| `/dashboard/orders` | **Client** | Session check via API | API calls | None | +| `/dashboard/orders/[id]` | **Client** | Session check via API | API calls | None | +| `/dashboard/inventory` | **Client** | Session check via API | API calls | None | +| `/dashboard/categories` | **Client** | Session check via API | API calls | None | +| `/dashboard/brands` | **Client** | Session check via API | API calls | None | +| `/dashboard/attributes` | **Client** | Session check via API | API calls | None | +| `/dashboard/reviews` | **Client** | Session check via API | API calls | None | +| `/dashboard/coupons` | **Client** | Session check via API | API calls | None | +| `/dashboard/customers` | **Client** | Session check via API | API calls | None | +| `/dashboard/analytics` | **Client** | Session check via API | API calls | None | +| `/dashboard/notifications` | **Client** | Session check via API | API calls | None | +| `/dashboard/integrations` | **Client** | Session check via API | API calls | None | +| `/dashboard/landing-pages` | **Client** | Session check via API | API calls | None | +| `/dashboard/visual-editor` | **Client** | Session check via API | API calls | None | +| `/dashboard/webhooks` | **Client** | Session check via API | API calls | None | +| `/dashboard/emails` | **Client** | Session check via API | API calls | None | +| `/dashboard/settings/payments` | **Client** | Session check via API | API calls | None | +| `/dashboard/subscriptions` | **Client** | Session check via API | API calls | None | +| `/dashboard/cart` | **Client** | Session check via API | API calls | None | + +### Storefront Pages (`/store/[slug]/*`) + +| Route | Rendering | Auth Protection | Data Fetching | Error Boundary | +|---|---|---|---|---| +| `/store/[slug]` (layout) | **Server** | None (public store) | Prisma direct | None | +| `/store/[slug]` (page) | **Server** | None | Prisma direct (products, categories) | None | +| `/store/[slug]/products` | **Server** | None | Prisma direct | None | +| `/store/[slug]/products/[productSlug]` | **Server** | None | Prisma direct | None | +| `/store/[slug]/categories` | **Server** | None | Prisma direct | None | +| `/store/[slug]/categories/[categorySlug]` | **Server** | None | Prisma direct | None | +| `/store/[slug]/cart` | **Client** | None | Local state | None | +| `/store/[slug]/checkout` | **Client** | None | API calls | None | +| `/store/[slug]/checkout/success` | **Client** | None | API calls | None | +| `/store/[slug]/checkout/failure` | **Client** | None | None | None | +| `/store/[slug]/checkout/cancel` | **Client** | None | None | None | +| `/store/[slug]/orders/view` | **Client** | None | API calls | None | +| `/store/[slug]/orders/track` | **Client** | None | API calls | None | + +### Other Pages + +| Route | Rendering | Auth Protection | Data Fetching | Error Boundary | +|---|---|---|---|---| +| `/` (home) | **Server** | None (landing page) | None | None | +| `/api-docs` | **Client** | None | Fetches OpenAPI spec | None | +| `/chat` | **Client** | Session check | API calls | None | +| `/track` | **Client** | None | API calls | None | +| `/track/[consignmentId]` | **Client** | None | API calls | None | +| `/track/order/[orderId]` | **Client** | None | API calls | None | +| `/settings` | **Client** | Session check | API calls | None | +| `/settings/billing` | **Client** | Session check | API calls | None | +| `/settings/ai` | **Client** | Session check | API calls | None | +| `/settings/api-tokens` | **Client** | Session check | API calls | None | +| `/settings/integrations/facebook` | **Client** | Session check | API calls | None | +| `/settings/stormpilot` | **Client** | Session check | API calls | None | +| `/team` | **Client** | Session check | API calls | None | +| `/onboarding` | **Client** | Session check | API calls | None | +| `/projects` | **Client** | Session check | API calls | None | +| `/stormpilot` | **Client** | Session check | API calls | None | +| `/payment/success` | **Client** | None | Query params | None | +| `/payment/cancelled` | **Client** | None | None | None | +| `/payment/error` | **Client** | None | None | None | +| `/checkout` | **Client** | None | API calls | None | +| `/checkout/success` | **Client** | None | API calls | None | +| `/checkout/failure` | **Client** | None | None | None | +| `/checkout/confirmation` | **Client** | None | API calls | None | +| `/store-not-found` | **Server** | None | None | None | +| `/lp/[storeSlug]/[pageSlug]` | **Server** | None | Prisma direct | None | +| `/lp/preview/[templateId]` | **Server** | None | Prisma direct | None | + +--- + +## 4. Security Findings + +### CRITICAL Issues + +#### 4.1 Unauthenticated Order Detail Access (IDOR) +**Route:** `GET /api/store/[slug]/orders/[orderId]` +**Severity:** CRITICAL +**Description:** This endpoint returns complete order details (items, prices, payment status, shipping address, customer PII) to anyone who provides a valid `orderId`. There is no authentication, no email/phone verification, and no rate limiting. Order IDs are CUIDs which, while not sequential, are not cryptographic secrets. +**Impact:** Customer PII exposure (name, email, phone, address, payment details). +**Recommendation:** Require email or phone verification (like `/api/orders/[id]` does for unauthenticated access), or generate time-limited signed URLs for order detail access. + +#### 4.2 Unauthenticated Invoice Download +**Route:** `GET /api/store/[slug]/orders/[orderId]/invoice` +**Severity:** CRITICAL +**Description:** Anyone can download an invoice PDF for any order if they know the order ID and store slug. +**Impact:** Financial data exposure. +**Recommendation:** Add email/phone verification or signed URL tokens. + +#### 4.3 Unauthenticated Payment Verification Mutation +**Route:** `POST /api/store/[slug]/orders/[orderId]/verify-payment` +**Severity:** HIGH +**Description:** This endpoint triggers SSLCommerz payment status queries and **mutates database records** (updates payment status, order status) without any authentication. Additionally, `GET` is aliased to `POST`, meaning a simple browser navigation or image tag could trigger it. +**Impact:** An attacker could trigger payment verification polling. While they can't forge a successful payment, they could cause unnecessary load on SSLCommerz API and database writes. +**Recommendation:** Add HMAC-signed tokens or session verification. Remove the GET->POST alias. + +### HIGH Issues + +#### 4.4 Payment Configuration Without Authorization +**Route:** `POST /api/payments/configurations` +**Severity:** HIGH +**Description:** Any authenticated org member (regardless of role) can create/update payment gateway configurations. No permission check, no Zod validation. The config object is passed directly to Prisma. +**Impact:** A member with `CUSTOMER_SUPPORT` role could modify payment gateway credentials. +**Recommendation:** Add `payments:manage` permission check and Zod validation for the config object. + +#### 4.5 SSLCommerz Payment Initiation Trusts Client Data +**Route:** `POST /api/payments/sslcommerz/initiate` +**Severity:** HIGH +**Description:** The `amount`, `organizationId`, and `storeId` are taken from the request body without server-side verification. An attacker could initiate a payment for a lower amount than the order total. +**Impact:** Revenue loss if payment amount doesn't match order amount. +**Recommendation:** Look up the order by `orderId` and use the server-side `totalAmount`. + +#### 4.6 SSRF via Chat Web Fetch +**Route:** `POST /api/chat/webfetch` +**Severity:** HIGH +**Description:** Allows authenticated users to fetch arbitrary URLs server-side. Could be used to scan internal networks, access cloud metadata endpoints (169.254.169.254), or exfiltrate data. +**Recommendation:** Implement URL allowlist/blocklist, block private IP ranges, and limit response size. + +#### 4.7 Chat Session Access Without Ownership Check +**Route:** `GET/PATCH/DELETE /api/chat/sessions/[sessionId]` +**Severity:** HIGH +**Description:** Users can access, modify, or delete any chat session by ID regardless of ownership. +**Recommendation:** Add `where: { userId: session.user.id }` to all session queries. + +### MEDIUM Issues + +#### 4.8 Inconsistent Auth Patterns +**Description:** ~30% of routes use manual `getServerSession()` + `checkPermission()` instead of `apiHandler()`. These routes miss CSRF protection, Content-Type validation, and request size limits that `apiHandler` provides. +**Affected Routes:** `orders/route.ts`, `orders/[id]/route.ts`, `orders/bulk/route.ts`, `orders/export/route.ts`, `products/export/route.ts`, `products/bulk/route.ts`, `inventory/bulk/route.ts`, `inventory/export/route.ts`, `payments/configurations/route.ts`, `payments/sslcommerz/initiate/route.ts` +**Recommendation:** Migrate all routes to use `apiHandler()`. + +#### 4.9 Debug Logging in Production +**Description:** Multiple route handlers contain `console.log` statements that log request bodies, user IDs, and internal state. +**Affected Routes:** `products/[id]/route.ts` (PATCH), `orders/[id]/route.ts` (PATCH), `orders/route.ts` (POST) +**Impact:** Sensitive data in production logs. +**Recommendation:** Remove or gate behind `NODE_ENV === 'development'`. + +#### 4.10 Missing Permission Checks on Bulk/Export Routes +**Description:** `orders/bulk`, `orders/export`, `products/bulk`, `products/export` authenticate the user but don't check specific permissions. +**Recommendation:** Add explicit permission checks. + +#### 4.11 Demo Store Creation Lacks Rate Limiting +**Route:** `POST /api/demo/create-store` +**Description:** Protected only by `NODE_ENV !== 'production'` check. No rate limiting. Could be abused to create unlimited stores in non-production environments. +**Recommendation:** Add rate limiting. + +#### 4.12 Memory Leak in SSE Endpoints +**Description:** `orders/stream/route.ts` uses a `Map` for store access caching that grows unbounded. +**Recommendation:** Add TTL eviction or use LRU cache. + +### LOW Issues + +#### 4.13 Inconsistent Response Formats +**Description:** Some routes return `{ error: string }`, others `{ success: boolean, error: string, code: string }`. The codebase has two response utility libraries (`api-middleware.ts` and `api-response.ts`) with different formats. +**Recommendation:** Standardize on one format. + +#### 4.14 Store Settings Returns Mock Data +**Route:** `GET /api/stores/[id]/settings` +**Description:** Returns hardcoded mock data instead of actual database values. +**Recommendation:** Implement actual database integration. + +#### 4.15 organizationId Exposed in Public Store Endpoint +**Route:** `GET /api/store/[slug]` +**Description:** Returns `organizationId` in the public response. While not directly exploitable, it provides internal ID information. +**Recommendation:** Remove `organizationId` from public response or assess if it's needed. + +#### 4.16 Missing Error Boundaries on Dashboard Pages +**Description:** Most dashboard pages are client-rendered without error boundaries. An uncaught error in any component will crash the entire page. +**Recommendation:** Add `` wrappers to all dashboard page components. + +--- + +## 5. Issues Summary + +### By Severity + +| Severity | Count | Key Concerns | +|---|---|---| +| **CRITICAL** | 3 | Unauthenticated order/invoice/payment access | +| **HIGH** | 4 | Payment config without RBAC, SSRF, session IDOR, client-trusted amounts | +| **MEDIUM** | 5 | Inconsistent auth patterns, debug logging, missing permissions, rate limiting, memory leak | +| **LOW** | 4 | Response format inconsistency, mock data, info leak, missing error boundaries | + +### By Category + +| Category | Issues | +|---|---| +| **Authentication** | 3 routes completely unauthenticated that should require some verification | +| **Authorization** | Payment config, chat sessions, bulk/export routes missing permission checks | +| **Input Validation** | Payment initiation trusts client amounts, several routes skip Zod validation | +| **CSRF** | ~30% of state-changing routes bypass CSRF protection by not using `apiHandler` | +| **Information Disclosure** | Debug logging, organizationId in public endpoints, full order details without auth | +| **Rate Limiting** | Demo store creation, verify-payment endpoint | +| **Code Quality** | Inconsistent patterns, mock data, memory leaks | + +### Top Priority Fixes + +1. **Add authentication/verification to** `GET /api/store/[slug]/orders/[orderId]` and `GET /api/store/[slug]/orders/[orderId]/invoice` +2. **Add auth + remove GET alias from** `POST /api/store/[slug]/orders/[orderId]/verify-payment` +3. **Add permission check to** `POST /api/payments/configurations` +4. **Server-side amount verification for** `POST /api/payments/sslcommerz/initiate` +5. **Add URL allowlist/blocklist to** `POST /api/chat/webfetch` +6. **Add ownership check to** `/api/chat/sessions/[sessionId]` +7. **Migrate all routes to `apiHandler()`** for consistent CSRF + auth + validation +8. **Remove debug `console.log` statements** from production code paths diff --git a/COMPONENT_CODE_REVIEW.md b/COMPONENT_CODE_REVIEW.md new file mode 100644 index 00000000..244f9c37 --- /dev/null +++ b/COMPONENT_CODE_REVIEW.md @@ -0,0 +1,523 @@ +# Comprehensive Component Code Review Report + +**Scope:** `/workspace/src/components/` — 272 component files across 36 directories +**Date:** April 1, 2026 +**Reviewer:** Automated deep analysis + +--- + +## Executive Summary + +The codebase is a **multi-tenant SaaS e-commerce platform** built with Next.js (App Router), React, TypeScript, Tailwind CSS, and Radix UI primitives. The UI library follows the shadcn/ui pattern. Overall code quality is **good**, with consistent patterns, strong TypeScript usage, and a well-structured component library. However, there are several issues across security, architecture, performance, and maintainability that should be addressed. + +**Total findings: 47** +- Critical: 5 +- High: 12 +- Medium: 18 +- Low: 12 + +--- + +## 1. CRITICAL FINDINGS + +### 1.1 [SECURITY] Checkout sends client-side prices to server without server-side validation guarantee + +**Files:** `checkout/payment-method-step.tsx`, `checkout/cart-review-step.tsx` + +The checkout flow sends `item.price` from the client-side cart store directly in the order creation payload: + +```typescript +// payment-method-step.tsx:47-52 +const itemsForOrder = items.map((item) => ({ + productId: item.productId, + variantId: item.variantId, + quantity: item.quantity, + price: item.price, // Client-controlled value +})); +``` + +While there is a `/api/checkout/validate` step, the actual order creation at `/api/checkout/complete` receives the client-provided price. If the server does not re-verify prices from the database, this is a **price manipulation vulnerability**. The component trusts the client store as the source of truth for pricing. + +**Recommendation:** Remove `price` from the client payload entirely. The server should look up current prices from the database at order creation time. + +--- + +### 1.2 [SECURITY] SSLCommerz payment sends empty `organizationId` and `storeId` + +**File:** `checkout/payment-method-step.tsx:223-234` + +```typescript + +``` + +Both `organizationId` and `storeId` are passed as empty strings. The SSLCommerz component sends these directly to the payment initiation API. Depending on server-side handling, this could cause payment association failures, orphaned payments, or cross-tenant payment leaks. + +**Recommendation:** Pass the actual `organizationId` and `storeId` from session/context, or have the server derive them from the authenticated session. + +--- + +### 1.3 [SECURITY] Stripe payment component is a non-functional placeholder that can mislead + +**File:** `checkout/stripe-payment.tsx` + +The Stripe payment component is an incomplete placeholder with no actual payment confirmation logic. It always calls `onError()` with a message that Stripe is not implemented, but the component is importable and renderable. If a developer or configuration accidentally enables this, users will see a "Pay" button that never works. + +Additionally, `console.log('Stripe loaded successfully')` reveals internal state to browser devtools. + +**Recommendation:** Either complete the Stripe integration or remove the component entirely to prevent dead-code confusion. If kept as a stub, add a build-time warning or disable the component via a feature flag. + +--- + +### 1.4 [SECURITY] `dangerouslySetInnerHTML` usage in storefront components + +**Files:** `storefront/hero-section.tsx`, `landing-pages/landing-page-renderer.tsx`, `emails/preview-email-dialog.tsx` + +Multiple storefront components use `dangerouslySetInnerHTML` to render content. If any of this content originates from user input (store owner configuration, landing page editor, email templates), this creates **XSS attack vectors**. + +**Recommendation:** Sanitize all HTML content with DOMPurify or a similar library before rendering via `dangerouslySetInnerHTML`. Add a shared utility function `sanitizeHtml()` for consistent application. + +--- + +### 1.5 [SECURITY] Native `confirm()` used for destructive admin actions + +**Files:** `subscription/admin/plan-management.tsx:183`, `subscriptions/subscriptions-list.tsx:128`, plus 12 more components + +```typescript +if (!confirm('Are you sure you want to delete this plan?')) return; +``` + +Native `confirm()` dialogs: +- Cannot be styled or branded +- Are blockable by browser settings +- Provide no CSRF protection +- Skip the application's accessibility layer + +For admin operations like deleting subscription plans or cancelling subscriptions, this is insufficient. + +**Recommendation:** Use the existing `AlertDialog` component (already used elsewhere in the codebase) for all destructive confirmations. + +--- + +## 2. HIGH SEVERITY FINDINGS + +### 2.1 [ARCHITECTURE] Duplicate components with divergent implementations + +**Duplicated pairs found:** + +| Component | Location 1 | Location 2 | +|-----------|-----------|-----------| +| `ProductQuickView` | `product-quick-view.tsx` (Tabler icons, `formatCurrency` from utils) | `products/product-quick-view.tsx` (Lucide icons, `formatMoney` from money) | +| `PriceRangeFilter` | `price-range-filter.tsx` (Tabler icons, `formatCurrency`) | `products/price-range-filter.tsx` (Lucide, `formatMoney`, debounce hook) | +| Notifications | `admin/notifications-list.tsx` | `notifications/notifications-list.tsx` | + +The two `ProductQuickView` components have **different interfaces** (`ProductQuickViewData` type differs), different icon libraries, and different formatting functions. This causes confusion for consumers and risks inconsistent behavior. + +**Recommendation:** Consolidate each pair into a single canonical component. Establish a convention: domain-specific components live in their feature directory, shared ones in a common location. + +--- + +### 2.2 [ARCHITECTURE] Inconsistent money formatting across components + +**Two competing patterns:** + +1. `formatCurrency` from `@/lib/utils` — used in 17 component files +2. `formatMoney` from `@/lib/money` — used in 25 component files + +Some files even use both or define their own inline: +```typescript +// Multiple components define this pattern: +const formatCurrency = formatMoney; // Alias for readability +``` + +Additionally, `admin-dashboard.tsx` formats revenue inline as `৳{(stats.revenue?.total || 0).toLocaleString()}`, bypassing both utilities. + +**Recommendation:** Standardize on `formatMoney` from `@/lib/money` (already the majority pattern). Remove `formatCurrency` from utils or make it re-export `formatMoney`. Fix inline formatting. + +--- + +### 2.3 [PERFORMANCE] `use-toast.ts` has a memory leak in listener cleanup + +**File:** `ui/use-toast.ts:167-175` + +```typescript +React.useEffect(() => { + listeners.push(setState); + return () => { + const index = listeners.indexOf(setState); + if (index > -1) { + listeners.splice(index, 1); + } + }; +}, [state]); // BUG: depends on `state` +``` + +The `useEffect` dependency is `[state]`, which means the cleanup and re-subscription runs on **every state change**. This causes unnecessary churn in the listeners array. With rapid toast updates, this could cause missed updates or stale closures. + +**Recommendation:** Change the dependency to `[]` (empty array) since `setState` identity is stable across renders. + +--- + +### 2.4 [ARCHITECTURE] Toast system duplication: Radix Toast vs Sonner + +The codebase has **two competing toast systems**: + +1. **Radix-based toast** (`ui/toast.tsx`, `ui/use-toast.ts`, `ui/toaster.tsx`) — custom implementation with reducer pattern +2. **Sonner** (`ui/sonner.tsx`) — third-party toast library, `import { toast } from 'sonner'` + +Sonner is used in the vast majority of components (checkout, subscription, admin, etc.), while the Radix toast system appears to be legacy/unused. + +**Recommendation:** Remove the Radix toast system (`toast.tsx`, `use-toast.ts`, `toaster.tsx`) if Sonner is the standard. This eliminates ~200 lines of dead code and removes consumer confusion. + +--- + +### 2.5 [PERFORMANCE] `ui/table.tsx` unnecessarily marked `"use client"` + +**File:** `ui/table.tsx` + +The Table component is a pure presentational component with no hooks, state, effects, or event handlers. It's marked `"use client"` despite being eligible as a Server Component. + +This forces the Table and all its sub-components into the client bundle, adding unnecessary JavaScript to every page that uses tables (admin, dashboard, billing, subscriptions). + +**Recommendation:** Remove `"use client"` from `table.tsx`. Similarly audit `card.tsx`, `badge.tsx`, and other stateless UI components. + +--- + +### 2.6 [PERFORMANCE] Large icon library imports from both Lucide and Tabler + +Components inconsistently import from two icon libraries: +- **Lucide React** (`lucide-react`) — used in most components +- **Tabler Icons** (`@tabler/icons-react`) — used in `enhanced-data-table.tsx`, `bulk-action-bar.tsx`, `price-range-filter.tsx`, `product-quick-view.tsx` + +Both are tree-shakeable, but having two icon libraries increases: +- Bundle size (duplicate SVG rendering infrastructure) +- Cognitive overhead for developers +- Inconsistent icon styling + +**Recommendation:** Standardize on one icon library. Lucide is already dominant; migrate the ~10 files using Tabler to Lucide. + +--- + +### 2.7 [ARCHITECTURE] Missing `"use client"` directive in several UI components + +**Files:** `ui/button.tsx`, `ui/card.tsx`, `ui/input.tsx`, `ui/loading-skeletons.tsx` + +These files don't have `"use client"` but are pure presentational components. While this is actually **correct** (they should be Server Components), they're imported by client components which may cause confusion. The inconsistency in the UI library — where some primitives are `"use client"` and others aren't — should be deliberate and documented. + +**Note:** `button.tsx` and `input.tsx` not having `"use client"` is **good practice** since they use no client-side APIs. This is flagged for awareness, not as a bug. + +--- + +### 2.8 [PERFORMANCE] Admin dashboard falls back to hardcoded mock data in production + +**File:** `admin/admin-dashboard.tsx:41` + +```typescript +} catch { + setStats(mockStats); + console.log('Using mock admin stats data'); +} +``` + +On API failure, the component silently renders **hardcoded mock data** showing 1,234 users, ৳125,678 revenue, etc. In production, this displays fake metrics to administrators without any indication they're looking at mock data. + +**Recommendation:** Show an error state with a retry button instead of mock data. If mock data is needed for development, gate it behind `process.env.NODE_ENV === 'development'`. + +--- + +### 2.9 [SECURITY] Shipping form defaults to `country: 'BD'` but country selector doesn't include Bangladesh + +**File:** `checkout/shipping-details-step.tsx` + +The form defaults to `country: 'BD'` (Bangladesh), and the Dhaka-detection logic hardcodes `data.city.toLowerCase().includes('dhaka')`. However, the country `