From 5b93b2d23ec197899cd5419dac6f44a601fbbf7f Mon Sep 17 00:00:00 2001 From: Siavash Sameni Date: Sun, 24 May 2026 08:03:20 +0400 Subject: [PATCH] docs: add comprehensive logical audit report Adds a full cross-document audit covering: - Data Models (broken refs, ghost states, missing constraints) - API Reference (unauthenticated endpoints, field mismatches, missing pagination) - Architecture (fictitious deps, statelessness claims vs reality) - Flows (race conditions, missing failure paths, auth bypasses) - Security (passkey stubs, JWT storage, webhook verification) 32 findings organized by severity with recommended fixes. --- .../Platform Logical Audit - 2026-05-24.md | 435 ++++++++++++++++++ 1 file changed, 435 insertions(+) create mode 100644 09 - Audits/Platform Logical Audit - 2026-05-24.md diff --git a/09 - Audits/Platform Logical Audit - 2026-05-24.md b/09 - Audits/Platform Logical Audit - 2026-05-24.md new file mode 100644 index 0000000..b321b26 --- /dev/null +++ b/09 - Audits/Platform Logical Audit - 2026-05-24.md @@ -0,0 +1,435 @@ +# Platform Logical Audit + +**Date:** 2026-05-24 +**Scope:** Data Models, API Reference, Architecture, Flows, Security +**Method:** Cross-document consistency check, state-machine validation, authorization gap analysis, dependency verification + +--- + +## Executive Summary + +This audit identifies critical logical contradictions, security holes, and cross-document inconsistencies across the Amn platform documentation. The most severe findings are: + +1. **Dispute/escrow race condition** allowing fund release while a dispute is active. +2. **Three mutually incompatible `Dispute` status/action enums** across the Data Model, API Reference, and Flow documents — the documented API cannot be implemented against the documented schema. +3. **Passkey (WebAuthn) implementation is cryptographically broken** and unusable in production. +4. **Multiple financial endpoints require no authentication**, allowing unauthenticated payment record injection and private data exfiltration. +5. **Web3 payment verification only checks `receipt.status`**, not recipient address or amount, making payment fraud trivial. + +--- + +## 🔴 Critical Issues + +### 1. Dispute Does Not Auto-Pause Escrow (Funds-at-Risk) + +**Files:** `04 - Flows/Dispute Flow.md`, `03 - API Reference/Dispute API.md` + +**Finding:** The Dispute Flow explicitly states: *"Today, opening a dispute does **not** flip `Payment.escrowState` away from `funded`. An admin could theoretically still release the escrow before resolving the dispute."* + +At the same time, the Dispute API claims dispute creation *"Pauses any in-flight payout (sets a hold flag)."* + +**Impact:** A buyer opens a dispute; an admin (or malicious/compromised admin) releases the escrow before resolution. The seller receives funds while the dispute is still open. + +**Required Fix:** Introduce a `disputed` escrow state (or `disputeHold` flag) that blocks all release/refund operations until the dispute is resolved. The hold must be enforced in `PaymentCoordinator`, not just in controller logic. + +--- + +### 2. Passkey Authentication Is Completely Broken + +**Files:** `04 - Flows/Passkey (WebAuthn) Flow.md`, `02 - Data Models/User.md` + +**Findings:** +- **Attestation is stubbed:** The `publicKey` field is stored as the literal string `'simulated-public-key'`. A malicious client can register any credential ID under any user account. +- **Refresh tokens are not persisted:** Passkey-issued refresh tokens are never appended to `user.refreshTokens[]`. Standard `/api/auth/refresh-token` will reject them, breaking session continuity. +- **In-memory challenge store:** `storedChallenges` is a `Map` in process memory. In a horizontally scaled deployment, the challenge created on instance A can only be verified on instance A. Load-balancer round-robin breaks authentication entirely. + +**Impact:** Passkey auth is trivially bypassable and non-functional at scale. + +**Required Fix:** Replace stub with `@simplewebauthn/server` (or equivalent), persist real public keys, store refresh tokens in the user record, and move challenges to Redis with TTL. + +--- + +### 3. Unauthenticated State-Mutation Endpoints + +**Files:** `03 - API Reference/Payment API.md`, `03 - API Reference/AI API.md`, `03 - API Reference/Notification API.md` + +**Findings:** + +| Endpoint | Risk | +|----------|------| +| `POST /api/payment/decentralized/save` | Anyone can persist a Web3 payment record | +| `POST /api/payment/decentralized/update` | Anyone can update decentralized payment status/confirmations | +| `GET /api/payment/decentralized/history/:userId` | Anyone can read any user's payment history (privacy breach) | +| `POST /api/payment/shkeeper/create-test-payment` | Anyone can inject test payment records into production data | +| `POST /api/payment/decentralized/verify/:paymentId` | Anyone can trigger chain re-verification | +| `POST /api/payment/decentralized/verify-all-pending` | Anyone can trigger a global batch verification job | +| All AI endpoints (`/generate`, `/analyze`, `/translate`, `/assist`) | No caller identity; unlimited OpenAI cost abuse | +| Legacy notification router (`notification/routes.ts`) | Mounted without auth; accepts `?userId=` query parameter allowing notification read/modify for any user | + +**Impact:** Data poisoning, privacy violations, and unbounded cost exposure. + +**Required Fix:** Add Bearer JWT middleware to all endpoints above. Enforce ownership or admin-role checks on `:userId` parameterized endpoints. + +--- + +### 4. Web3 Payment Verification Trusts the Wrong Data + +**Files:** `04 - Flows/Payment Flow - DePay & Web3.md` + +**Finding:** The verifier checks only `receipt.status === '0x1'`. It **does not** decode the `Transfer` event to verify: +- `to` address == `ESCROW_WALLET_ADDRESS` +- `value` >= expectedAmount (accounting for decimals) + +**Impact:** A malicious actor can submit the hash of any successful transaction (e.g., a 0.01 USDT transfer to their own wallet) and the system will mark the payment as completed. + +**Required Fix:** Decode the event logs; verify recipient, token contract address, and amount match the intent. + +--- + +### 5. Three Mutually Incompatible Dispute Enum Sets + +**Files:** `02 - Data Models/Dispute.md`, `03 - API Reference/Dispute API.md`, `04 - Flows/Dispute Flow.md` + +**Finding:** The three documents describe three different `Dispute` systems: + +| Source | Status Enum | Resolution Action Enum | +|--------|-------------|------------------------| +| **Data Model** | `pending`, `in_progress`, `waiting_response`, `resolved`, `rejected`, `closed` | `refund`, `replacement`, `compensation`, `warning_seller`, `ban_seller`, `no_action` | +| **API Reference** | `open`, `under_review`, `resolved_buyer`, `resolved_seller`, `closed` | `buyer`, `seller`, `split` | +| **Flow** | `pending`, `in_progress`, `resolved`, `closed` | `refund`, `partial`, `release`, `reject` | + +**Impact:** The API cannot be implemented as documented against the data model. The request/response schemas for `POST /api/disputes/:id/resolve` are completely different between API (uses `decision`) and Flow (uses `action`). + +**Required Fix:** Create a single source-of-truth enum set. Align all three documents (or generate API docs from the model). `ban_seller` also has no corresponding `User.status` value (`User.status` is `active | suspended | deleted`). + +--- + +## 🟠 High Severity Issues + +### 6. Payment Model Uses `Mixed` for Core Foreign Keys + +**File:** `02 - Data Models/Payment.md` + +`purchaseRequestId`, `sellerOfferId`, and `sellerId` are `Schema.Types.Mixed` (ObjectId *or* String) to support the template flow. This sacrifices all type safety, referential integrity, and indexing efficiency for the normal (non-template) payment path, which is the vast majority of transactions. + +**Fix:** Use strict `ObjectId` refs for the standard path; store template linkage in a separate `metadata` field. + +--- + +### 7. Broken Foreign Key References + +**Files:** `02 - Data Models/PointTransaction.md`, `02 - Data Models/Chat.md` + +- `PointTransaction.order` references an **`Order` model that does not exist** in any documented schema. +- `Chat.relatedTo.type` includes `Transaction`, but no `Transaction` model exists. + +**Fix:** Remove or rename orphaned references. If `Order` is an internal alias, document it. + +--- + +### 8. Delivery Confirmation Code Is Brute-Forceable + +**Files:** `04 - Flows/Delivery Confirmation Flow.md` + +The 6-digit code (`Math.floor(100000 + Math.random()*900000)`) has only 900,000 combinations. There is **no rate limiting** documented on verification attempts. + +**Fix:** Add Redis-backed rate limiting (e.g., 5 attempts per 15 minutes per request). Consider increasing entropy or adding time-based invalidation. + +--- + +### 9. Purchase Request Status Machine Is Inconsistent + +**Files:** `02 - Data Models/PurchaseRequest.md`, `03 - API Reference/Marketplace API.md`, `04 - Flows/Purchase Request Flow.md` + +Three different status enums: + +| Source | Unique Values | +|--------|---------------| +| **Data Model** | `pending_payment`, `active`, `seller_paid` | +| **API** | `draft` | +| **Flow** | `finalized`, `archived` | + +The flow also claims a backward transition `in_negotiation → received_offers` is valid, but states elsewhere that "progression is forward-only except into terminal status." + +**Fix:** One canonical enum. Remove ghost states (`draft`, `finalized`, `archived`, `pending_payment`, `active`) or implement them consistently. + +--- + +### 10. JWTs in `localStorage` with 7-Day Expiry + +**Files:** `01 - Architecture/Security Architecture.md` + +Access tokens are stored in `localStorage` (XSS theft vector) and expire in **7 days**. For a financial escrow platform, this is far too long. The docs admit the risk but only list secondary mitigations (CSP, audits). + +**Fix:** Move tokens to `httpOnly` cookies with 15–60 minute expiry and implement a secure refresh-token rotation mechanism. + +--- + +### 11. SHKeeper Webhook Swallows All Errors + +**File:** `04 - Flows/Payment Flow - SHKeeper.md` + +The webhook returns `202 Accepted` even on signature failures, DB errors, and unknown payments. Failures are silently swallowed with no dead-letter queue, retry mechanism, or alerting. + +**Fix:** Differentiate `400` (client error, do not retry) from `500` (server error, retry). Log structured webhook failures to a DLQ or alerting channel. + +--- + +### 12. Socket.IO Room Joining Is Client-Controlled + +**Files:** `01 - Architecture/Real-time Layer.md` + +Clients explicitly emit `join-user-room {userId}` with their own ID. The docs include a warning that `userId` arguments "are NOT trusted blindly" and add "(cite: needs verification in `socketService.ts`)." This is an explicit admission the authorization check may not be implemented. If missing, any authenticated user can subscribe to another user's private notifications. + +**Fix:** Remove client-driven `join-user-room` events. The server should automatically join the socket to `user-{decoded.id}` immediately after handshake auth. + +--- + +### 13. Rate Limiting Is Effectively Disabled + +**Files:** `01 - Architecture/Backend Architecture.md`, `03 - API Reference/API Overview.md` + +`express-rate-limit` is disabled by default. Protected paths are limited to auth OTP/resend and AI endpoints. Marketplace, payment, chat, file upload, and notification endpoints have **no documented rate limiting**. + +**Fix:** Enable global rate limiting with tiered limits (stricter for auth/financial, moderate for chat/marketplace). + +--- + +## 🟡 Medium Severity Issues + +### 14. Architecture Claims Stateless, But Isn't + +**Files:** `01 - Architecture/System Architecture.md`, `01 - Architecture/Backend Architecture.md`, `01 - Architecture/Real-time Layer.md` + +- Claims backend is **stateless** (JWT-only, no sessions). +- Admits Redis stores **login-attempt lockout counters** (session state). +- Socket.IO requires **sticky sessions** for multi-node, but current infra is a single Docker host. Running `N=2` backend replicas would break real-time events. + +**Fix:** Remove the "stateless" claim until auth state and Socket.IO are node-agnostic (Redis adapter + cookie-based sticky sessions). + +--- + +### 15. Fictitious Dependencies + +**Files:** `01 - Architecture/Frontend Architecture.md`, `01 - Architecture/Infrastructure.md` + +- **Next.js 16** does not exist (latest stable is 15). Using an unverified framework version for a financial platform is high-risk. +- **Redis 8** (`redis:8-alpine`) does not exist (latest stable is 7.x). + +**Fix:** Update docs to reflect actual tested versions. + +--- + +### 16. Blog Post `viewsCount` Incremented on GET + +**File:** `03 - API Reference/Blog API.md` + +`GET /api/blog/posts/:slug` atomically increments `viewsCount`. GET requests should be idempotent and safe. + +**Fix:** Move view counting to a `POST /api/blog/posts/:slug/view` beacon endpoint, or use an async analytics pipeline. + +--- + +### 17. API Request/Response Fields Do Not Match Data Models + +**File:** `03 - API Reference/*` + +| API | Uses | Model Has | +|-----|------|-----------| +| Address (`POST /api/addresses`) | `fullName`, `street`, `postalCode`, `isPrimary` | `name`, `fullAddress`, `zipCode`, `primary` | +| Blog (`POST /api/blog/posts`) | `excerpt`, `isFeatured`, `videoUrl`, `metadata` | `description`, `featured`, `videos[]`, no `metadata` | +| Chat (`POST /api/chat`) | `title` | `name` | +| Chat message (`POST /api/chat/:id/messages`) | `type`, `replyToMessageId` | `messageType`, `replyTo` | +| Payment (multiple) | flat `amount: number` | nested `amount: { amount, currency }` | +| Notification (`POST /api/notifications`) | `body`, `type` (domain) | `message`, `type` (severity), `category` (domain) | +| User profile (`PUT /api/user/profile`) | `name` (alias for `profile.name`) | No `profile.name` field exists | +| Points (`POST /api/points/admin/add`) | `reason` | `description` (required) | + +**Fix:** Align API specs to the data models, or vice versa, with a single source of truth. + +--- + +### 18. Seller Can Update Price After Offer Acceptance + +**File:** `04 - Flows/Negotiation Flow.md` + +The flow admits: *"`updateOffer` does not enforce status... Current code allows the price change, which is dangerous post-payment."* + +**Fix:** Reject `updateOffer` if `status !== 'pending'`. + +--- + +### 19. Buyer Can Confirm Delivery Before Seller Ships + +**File:** `04 - Flows/Delivery Confirmation Flow.md` + +Preconditions list `payment`, `processing`, or `delivery`. The "manual fast-track" lets the buyer skip the code and confirm receipt at any time — including immediately after payment, before the seller acknowledges. + +**Fix:** Restrict fast-track confirmation to status `delivery` only, or require admin override. + +--- + +### 20. Payment `confirmed` Status Is a Ghost State + +**Files:** `02 - Data Models/Payment.md`, `04 - Flows/Payment Flow - SHKeeper.md` + +The model includes `confirmed` in `status`. The SHKeeper webhook maps `PAID` directly to `completed`, skipping `confirmed`. No automated flow appears to set it. + +**Fix:** Remove `confirmed` from the enum, or document the transition that sets it. + +--- + +### 21. `partial` Escrow State Does Not Exist in Model + +**Files:** `04 - Flows/Escrow Flow.md`, `04 - Flows/Payment Flow - SHKeeper.md`, `02 - Data Models/Payment.md` + +Both flows use `escrowState: "partial"` for overpayments. The `Payment` model enum is `funded | releasable | released | refunded | releasing | failed` — `partial` is absent. + +**Fix:** Add `partial` to the model enum, or map overpayments to `funded` with a `overpaidAmount` metadata field. + +--- + +### 22. `BlogPost.comments` Is Just a Number + +**File:** `02 - Data Models/BlogPost.md` + +There is no `Comment` model. The system counts comments but cannot store or retrieve actual comment content. + +**Fix:** Implement a `Comment` collection, or remove the `comments` counter. + +--- + +### 23. No Pagination on List-Oriented Endpoints + +**File:** `03 - API Reference/*` + +- `GET /api/marketplace/sellers` +- `GET /api/marketplace/purchase-requests/my` +- `GET /api/marketplace/purchase-requests/:id/offers` +- `GET /api/blog/posts/featured` +- `GET /api/points/leaderboard` +- `GET /api/users/contacts` + +**Fix:** Add cursor- or offset-based pagination to all unbounded list endpoints. + +--- + +### 24. Missing Array Defaults + +**Files:** `02 - Data Models/*` + +Across 5+ models, array fields (`offers[]`, `tags[]`, `attachments[]`, `evidence[]`, `timeline[]`, `participants[]`, etc.) have no default value. In Mongoose they default to `undefined`, causing runtime errors on `.push()` or `.length`. + +**Fix:** Add `default: []` to all array fields. + +--- + +### 25. Inconsistent Soft-Delete Patterns + +**Files:** `02 - Data Models/*` + +| Model | Pattern | +|-------|---------| +| User | `status: active / suspended / deleted` | +| BlogPost | `status: draft / published / archived` | +| RequestTemplate, Category, LevelConfig | `isActive` boolean | +| Dispute | `status` enum (no deleted state) | +| SellerOffer | `status` enum | + +**Fix:** Standardize on one pattern (e.g., `status` enum with `deleted` or `archived`, plus `deletedAt` timestamp). + +--- + +### 26. Frontend Docker Image Cannot Be Promoted Across Environments + +**File:** `01 - Architecture/Frontend Architecture.md` + +The frontend uses `process.env.NEXT_PUBLIC_API_URL`, which is baked at **build time**. The same Docker image cannot be promoted from dev → staging → prod without rebuilding. + +**Fix:** Inject the API URL at runtime via a server-side config endpoint or an env-replacement script in the container entrypoint. + +--- + +### 27. Watchtower Auto-Deploys with Zero Staging Gate + +**File:** `01 - Architecture/Infrastructure.md` + +Production auto-updates every 5 minutes on `latest` tag change with no health-check gate, blue/green rollout, or smoke test. + +**Fix:** Implement a staging promotion pipeline: build → deploy to staging → smoke test → tag promote to `stable` → Watchtower watches `stable`, not `latest`. + +--- + +## 🟢 Lower Severity / Polish Issues + +### 28. Naming Inconsistencies + +**Files:** `02 - Data Models/*` + +- `PointTransaction.user` vs `*Id` suffix convention used everywhere else +- `User.referredBy` vs `referredById` +- `User.profile.phone` vs `phoneNumber` used in `Address` and `PurchaseRequest` +- `Chat.messages[].senderType` defaults to `User` (PascalCase) while 95% of enums use lowercase +- `Address.addressType` uses `Home` / `Office` / `Other` (PascalCase) +- `Chat.metadata.createdBy` lacks `Id` suffix +- `Dispute.resolution.resolvedBy` lacks `Id` suffix + +### 29. Redundant / Derived Fields + +**Files:** `02 - Data Models/PurchaseRequest.md`, `02 - Data Models/User.md` + +- `PurchaseRequest.deliveryConfirmed` boolean + `deliveryConfirmedAt` — boolean is derivable +- `PurchaseRequest.deliveryInfo.deliveryCodeUsed` boolean — derivable from `deliveryCodeUsedAt` +- `User.points.total` — derivable from `available + used`; risks arithmetic drift +- `PurchaseRequest.rating` + `feedback` — duplicate the dedicated `Review` model (two sources of truth) + +### 30. ER Diagram Errors + +**File:** `02 - Data Models/Data Model Overview.md` + +- `PURCHASE_REQUEST ||--o| REVIEW` implies max one review; the model allows many +- `CHAT ||--o{ DISPUTE` direction is reversed; a chat belongs to at most one dispute + +### 31. Missing Audit Timestamps + +**Files:** `02 - Data Models/*` + +- `User` has `status: deleted` but no `deletedAt` +- `PurchaseRequest` has terminal states (`completed`, `seller_paid`, `cancelled`) but no `completedAt` / `cancelledAt` +- `Review` has `pending → published → rejected` workflow but no transition timestamps +- `Dispute` has `adminId` assignment but no `assignedAt` + +### 32. Missing Constraints + +**Files:** `02 - Data Models/*` + +- `Dispute.purchaseRequestId` has no unique constraint (allows duplicate active disputes per request) +- `PurchaseRequest.deliveryInfo.deliveryCode` is not unique or sparse-unique across requests +- `Category.parentId` has no circular-reference guard +- `User.referredBy` can self-reference with no prevention +- `LevelConfig.discountPercent` has no upper bound (`max: 100`) +- `RequestTemplate.expiresAt` can be created in the past + +--- + +## Cross-Cutting Themes + +1. **Documentation Drift:** The four primary document layers (Models, API, Flows, Architecture) describe different versions of the same system. The most egregious example is the `Dispute` entity, which has three incompatible definitions. +2. **Security Theater:** Several sections describe security measures (CSP, audits, future ClamAV) while fundamental flaws (localStorage tokens, unauthenticated endpoints, stubbed crypto) remain unaddressed. +3. **Ghost States:** Multiple status enums contain values that no automated flow sets (`confirmed`, `partial`, `finalized`, `archived`, `active` for SellerOffer). +4. **Operational Fragility:** The production topology is a single Docker host with automatic `latest` deployment. There is no HA, no staging gate, and no upload storage that can survive horizontal scaling (uploads are host bind-mounts). + +--- + +## Recommendations (Priority Order) + +1. **Fix the Dispute ↔ Escrow race condition** immediately. A `disputed` state must block all releases. +2. **Align all status enums** across Models, API, and Flows. Create a single source-of-truth document. +3. **Harden Passkey authentication** before any production use. +4. **Add authentication and ownership checks** to all financial endpoints. +5. **Fix Web3 verification** to decode and validate `Transfer` event parameters. +6. **Enable rate limiting globally**, with stricter tiers for auth and financial paths. +7. **Implement idempotency keys** for dispute creation, payment verify, and payout creation. +8. **Protect `/uploads`** with signed URLs or session-based auth for sensitive attachments. +9. **Fix SHKeeper webhook error handling** — return proper status codes and log to a DLQ. +10. **Sanitize logs** to remove plaintext verification/reset codes.