audit: 2026-05-30 full-codebase audit — report, issues, docs, runbooks
Full-codebase-audit 2026-05-30 outputs: - Audit report: 09 - Audits/Full Codebase Audit - 2026-05-30.md - 81 issue files ISSUE-055..135 (decisions + 1 skipped no-brainer). - Scanner docs from scratch (was zero): architecture, data model, API ref, payment flow, operations runbook + repo README. - Doc-sync updates across API reference, data models, flows, design system. - Secret Rotation Runbook (08 - Operations) for the exposed credentials. - Reusable workflow guide (07 - Development) + .claude/workflows/full-codebase-audit.js. Issues remain status:open intentionally — the code fixes are uncommitted-then-committed working-tree changes per repo and aren't "resolved" until merged/deployed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Siavash Sameni
@@ -12,7 +12,8 @@ audit: "2026-05-29 — corrected against source: Dispute.ts, DisputeService.ts,
|
||||
|
||||
When something goes wrong (item not delivered, wrong item, seller misbehaviour), either party can open a **dispute**. A three-way chat (buyer, seller, admin mediator) is created automatically. After evidence is gathered, the admin **resolves** the dispute — selecting an action such as refund, replacement, compensation, warning, or ban.
|
||||
|
||||
> [!danger] SECURITY — Three open privilege-escalation bugs exist as of this audit. See [Security Gaps](#security-gaps) below.
|
||||
> [!success] Security fixes applied (2026-05-30)
|
||||
> The three privilege-escalation bugs documented in the original Security Gaps section were fixed in commit `1d881c5` (ISSUE-003, ISSUE-004) and `fce8a19` (resolver role). Role guards are now enforced on assign/status/resolve; route shadowing is eliminated by remounting the release-hold router at `/api/disputes/pr`. See [Security Gaps](#security-gaps) for the historical record and current state.
|
||||
|
||||
> [!warning] Real-time events not implemented
|
||||
> Every Socket.IO emit in `DisputeService` is currently commented out. No `dispute-updated`, `new-notification`, or any other socket event fires for dispute creation, admin assignment, status changes, evidence uploads, or resolution. The dispute feature is CRUD-only at this stage.
|
||||
@@ -23,7 +24,8 @@ When something goes wrong (item not delivered, wrong item, seller misbehaviour),
|
||||
- **Seller** — party against whom the dispute is raised (or in rarer cases, initiator).
|
||||
- **Admin / Mediator** — assigned to investigate.
|
||||
- **Frontend** — buyer/seller "Report issue" buttons in the request detail view; admin dispute dashboard.
|
||||
- **Backend** — `DisputeService` (`backend/src/services/dispute/DisputeService.ts`), dashboard/controller routes at `backend/src/routes/disputeRoutes.ts` (mounted first at `/api/disputes`), and release-hold helpers in `backend/src/services/dispute/disputeRoutes.ts` (mounted second at `/api/disputes`).
|
||||
- **Admin / Mediator** — assigned to investigate (role `admin` or `resolver`).
|
||||
- **Backend** — `DisputeService` (`backend/src/services/dispute/DisputeService.ts`), dashboard/controller routes at `backend/src/routes/disputeRoutes.ts` (mounted at `/api/disputes`), and release-hold helpers in `backend/src/services/dispute/disputeRoutes.ts` (mounted at `/api/disputes/pr` since commit `1d881c5`).
|
||||
- **MongoDB** — `disputes`, `chats`, `purchaserequests`, `payments`.
|
||||
- **Socket.IO** — no events fire today; all emits are TODO stubs (see warning above).
|
||||
|
||||
@@ -78,59 +80,40 @@ Valid values: `product_quality | delivery_delay | wrong_item | payment_issue | s
|
||||
|
||||
---
|
||||
|
||||
## Security Gaps
|
||||
## Security Gaps (Historical — All Closed as of 2026-05-30)
|
||||
|
||||
### 1. `PATCH /api/disputes/:id/status` — no role guard
|
||||
The following bugs were identified in the 2026-05-29 audit and fixed in commits `1d881c5` and `fce8a19`. The descriptions below are preserved for historical reference and audit trail.
|
||||
|
||||
**File:** `backend/src/routes/disputeRoutes.ts` line 26
|
||||
### 1. `PATCH /api/disputes/:id/status` — no role guard ✅ FIXED
|
||||
|
||||
```ts
|
||||
router.patch('/:id/status', DisputeController.updateStatus);
|
||||
```
|
||||
**Fix:** `authorizeRoles('admin', 'resolver')` added in commit `fce8a19`. Only admins and resolvers can change dispute status.
|
||||
|
||||
Despite comments in the router saying "admin only", there is **no `authorizeRoles` middleware**. Any authenticated buyer or seller can call this endpoint and change a dispute's status to `resolved` or `closed`, bypassing the admin resolution flow entirely. This is an open privilege-escalation bug.
|
||||
### 2. `POST /api/disputes/:id/resolve` (dashboard router) — no role guard ✅ FIXED
|
||||
|
||||
### 2. `POST /api/disputes/:id/resolve` (dashboard router) — no role guard
|
||||
**Fix:** `authorizeRoles('admin', 'resolver')` added in commit `fce8a19`. Only admins and resolvers can resolve disputes.
|
||||
|
||||
**File:** `backend/src/routes/disputeRoutes.ts` line 29
|
||||
**Additional fix (ISSUE-004, commit `1d881c5`):** `DisputeService.resolveDispute` now calls `releaseHoldResolve()` on the linked `purchaseRequestId`, clearing the escrow hold automatically so the payment release is unblocked after resolution.
|
||||
|
||||
```ts
|
||||
router.post('/:id/resolve', DisputeController.resolveDispute);
|
||||
```
|
||||
### 3. `POST /api/disputes/:id/assign` — no role guard ✅ FIXED
|
||||
|
||||
No role guard. Any authenticated user can post a resolution — including `action: 'ban_seller'`. Note that the **release-hold router's** `POST /:purchaseRequestId/resolve` (`backend/src/services/dispute/disputeRoutes.ts` line 77) **does** correctly apply `authorizeRoles('admin')`. The dashboard router's resolve endpoint does not.
|
||||
|
||||
### 3. `POST /api/disputes/:id/assign` — no role guard
|
||||
|
||||
**File:** `backend/src/routes/disputeRoutes.ts` line 23
|
||||
|
||||
```ts
|
||||
router.post('/:id/assign', DisputeController.assignAdmin);
|
||||
```
|
||||
|
||||
Any authenticated user can call this with their own user ID in `{ adminId }` and self-assign as mediator for any dispute.
|
||||
**Fix:** `authorizeRoles('admin', 'resolver')` added in commit `fce8a19`. Only admins and resolvers can assign mediators.
|
||||
|
||||
---
|
||||
|
||||
## Route Shadowing
|
||||
## Route Shadowing (Historical — Resolved as of 2026-05-30)
|
||||
|
||||
Both routers are mounted at `/api/disputes` in `app.ts`:
|
||||
Previously both routers were mounted at `/api/disputes`, causing the dashboard router to intercept release-hold requests. Fixed in commit `1d881c5` (ISSUE-003):
|
||||
|
||||
```ts
|
||||
// app.ts line 521 — mounted FIRST
|
||||
// app.ts — current state
|
||||
app.use("/api/disputes", dashboardDisputeRoutes); // src/routes/disputeRoutes.ts
|
||||
|
||||
// app.ts line 585 — mounted SECOND
|
||||
app.use("/api/disputes", disputeRoutes); // src/services/dispute/disputeRoutes.ts
|
||||
app.use("/api/disputes/pr", disputeRoutes); // src/services/dispute/disputeRoutes.ts — new prefix
|
||||
```
|
||||
|
||||
Express evaluates routes in registration order. This creates two concrete hazards:
|
||||
|
||||
1. **`POST /api/disputes/:id/resolve`** — the dashboard router (mounted first) exposes `POST /:id/resolve` with no role guard. A request intended for the release-hold router's `POST /:purchaseRequestId/resolve` (which **does** require admin) will be intercepted and handled by the wrong, unguarded handler when a matching dispute `_id` is supplied.
|
||||
|
||||
2. **`POST /api/disputes/:purchaseRequestId/raise`** — this route exists only in the second (release-hold) router. It will be reached correctly only if the dashboard router does not first match the path. Since the dashboard router has no `/raise` route, requests pass through. However, as more routes are added to either router, collisions will grow silently.
|
||||
|
||||
**Recommendation:** Separate the two routers onto distinct path prefixes (e.g. `/api/disputes` for the dashboard controller, `/api/disputes/hold` for the release-hold service).
|
||||
Release-hold endpoints now use the `/api/disputes/pr/` prefix:
|
||||
- `POST /api/disputes/pr/:purchaseRequestId/raise`
|
||||
- `GET /api/disputes/pr/:purchaseRequestId/status`
|
||||
- `POST /api/disputes/pr/:purchaseRequestId/resolve`
|
||||
|
||||
---
|
||||
|
||||
@@ -171,7 +154,7 @@ Express evaluates routes in registration order. This creates two concrete hazard
|
||||
9. All three parties chat in the dispute chat room (same socket mechanics as [[Chat Flow]]). Each party can upload more evidence via `POST /api/disputes/:id/evidence` — `DisputeService.addEvidence` (`:305-337`) appends to `dispute.evidence[]` and writes a `timeline` entry `evidence_added`. **No socket event fires for evidence uploads.**
|
||||
10. The admin may also `PATCH /api/disputes/:id/status` with intermediate states or notes; this updates `dispute.status` and writes a `timeline` entry `status_changed`. **No socket event fires.**
|
||||
|
||||
> [!danger] `PATCH /api/disputes/:id/status` has no role guard — any authenticated user can change dispute status (see [Security Gaps](#security-gaps)).
|
||||
> [!note] `PATCH /api/disputes/:id/status` now requires `admin` or `resolver` role (`authorizeRoles('admin', 'resolver')`, commit `fce8a19`). The previously open privilege-escalation gap is closed.
|
||||
|
||||
### Phase 4 — Resolution
|
||||
|
||||
@@ -190,10 +173,11 @@ Express evaluates routes in registration order. This creates two concrete hazard
|
||||
- `dispute.closedAt = now`
|
||||
- Appends `timeline` entry `dispute_resolved`.
|
||||
- Saves.
|
||||
- **Calls `releaseHoldResolve(purchaseRequestId)`** — this clears the escrow hold automatically so the payment release is unblocked (ISSUE-004 fix, commit `1d881c5`).
|
||||
- **No socket event fires.** (`// TODO: Send notifications via Socket.IO`)
|
||||
13. **Financial side-effect (manual today):** depending on the action, the admin then triggers either the **release** ([[Payout Flow]] / [[Escrow Flow]]) or the **refund** as a separate step. The dispute service records the resolution; full automatic dispatch through the release/refund policy engine is still a hardening item.
|
||||
13. **Financial side-effect:** as of commit `1d881c5` the escrow hold is cleared automatically on resolution. The admin still needs to separately trigger the ledger-gated release ([[Payout Flow]] / [[Escrow Flow]]) or refund for actual fund movement.
|
||||
|
||||
> [!danger] `POST /api/disputes/:id/resolve` (dashboard router) has no role guard — any authenticated user can post any resolution action including `ban_seller` (see [Security Gaps](#security-gaps)).
|
||||
> [!note] `POST /api/disputes/:id/resolve` now requires `admin` or `resolver` role (`authorizeRoles('admin', 'resolver')`, commit `fce8a19`). The previously open privilege-escalation gap is closed.
|
||||
|
||||
---
|
||||
|
||||
|
||||
Reference in New Issue
Block a user