docs: start security performance logic audit

09 - Audits/Security DB Performance Logic Audit - 2026-06-07.md

@@ -0,0 +1,200 @@
+---
+title: Security, DB Performance, and Logic Audit - 2026-06-07
+tags: [audit, security, database, performance, logic, findings]
+created: 2026-06-07
+updated: 2026-06-07
+status: open
+---
+
+# Security, DB Performance, and Logic Audit - 2026-06-07
+
+Fresh post-remediation audit track after the DB Query & Schema Audit was closed. This pass targets security, authorization, sensitive data handling, DB/performance regressions outside the closed report, and business-logic/state-machine correctness.
+
+> [!info] Scope
+> Initial source pass covered mounted backend routes, auth/ownership middleware, uploads/static file serving, dispute controllers/services, delivery-code flows, frontend token handling, and template checkout batch paths. This document is the starting register; expand it as each wave verifies more surfaces.
+
+## Method
+
+- Review only code that is mounted or imported by mounted routes, unless the finding explicitly calls out legacy/dead code.
+- Separate confirmed findings from follow-up scan queue items.
+- Use the same severity posture as the DB audit:
+  - Critical: cross-tenant privilege bypass, funds/control-plane compromise, or exploitable unauthenticated secret exposure.
+  - High: authenticated cross-tenant mutation/read, sensitive token/code leakage, unbounded user-triggered load, or logic that can corrupt user-visible state.
+  - Medium: broken route behavior, false-deny authorization, latent risk, degraded performance, or cleanup needed before the next refactor.
+  - Low: hardening and maintainability issues with limited user impact.
+
+## Initial Summary
+
+| Severity | Count |
+|---|---:|
+| Critical | 0 |
+| High | 6 |
+| Medium | 4 |
+| Low | 0 |
+| Total | 10 |
+
+## High Findings
+
+### H1 - Dashboard dispute creation does not verify purchase-request ownership
+
+**Files:** `backend/src/routes/disputeRoutes.ts:10-11`, `backend/src/controllers/disputeController.ts:9-31`, `backend/src/services/dispute/DisputeService.ts:96-170`
+
+**Status:** Open
+
+`POST /api/disputes` is mounted under `/api/disputes` and only requires authentication. The controller correctly ignores the client-submitted `buyerId` and passes `req.user.id`, but `DisputeService.createDispute` only verifies that `purchaseRequestId` exists. It does not verify that the authenticated user is the purchase request buyer, seller, admin, or resolver before creating a dispute and dispute chat tied to that purchase request.
+
+An authenticated user can therefore create a dashboard dispute for another user's purchase request if they know or can infer the request id. The new dispute stores the attacker as `buyerId` and can pull the real selected/preferred seller into a chat.
+
+**Impact:** Cross-tenant state mutation, support queue pollution, seller notification/chat creation for unrelated requests, and possible confusion in admin dispute handling.
+
+**Fix:** Before creating the chat or dispute, load the purchase request context and require one of:
+
+- requester is the request buyer;
+- requester is the selected/preferred seller, if seller-initiated disputes are intentionally allowed;
+- requester is `admin` or `resolver`.
+
+Add a regression test where user B attempts to dispute user A's purchase request and receives 403.
+
+### H2 - Delivery codes are printed in backend logs
+
+**Files:** `backend/src/services/marketplace/marketplaceController.ts:2022-2025`, `backend/src/services/delivery/DeliveryService.ts:133-147`, legacy `backend/src/services/marketplace/routes.ts:2941-2947`
+
+**Status:** Open
+
+Delivery codes are sensitive one-time confirmation values. The mounted controller logs the full generated code:
+
+```ts
+console.log(`Delivery code generated for request ${id}: ${deliveryCode}`);
+```
+
+`DeliveryService.verifyDeliveryCode` also logs failed attempts with both the submitted value and the expected code.
+
+**Impact:** Anyone with backend log access can recover or validate a delivery code and influence delivery confirmation. The failed-attempt log is especially risky because it turns an invalid guess into the correct code in logs.
+
+**Fix:** Never log the code or expected value. Log only request id, actor id, success/failure reason, and an event id. If auditability is needed, store a salted hash or redacted code suffix only.
+
+### H3 - Google OAuth and bearer-token debug logs expose access-token material in development logs
+
+**Files:** `frontend/src/auth/services/google-oauth.ts:117-121`, `frontend/src/auth/services/google-oauth.ts:185-193`, `frontend/src/lib/axios.ts:69-76`, `frontend/src/utils/logger.ts:29-34`
+
+**Status:** Open
+
+The frontend logger is development-enabled by default. Google signup logs the full OAuth `tokenResponse` and then the full `access_token`. Google signin logs token metadata and a token prefix. The axios request interceptor logs a bearer-token preview and localStorage key names.
+
+**Impact:** Dev/local screenshots, browser consoles, test logs, or shared debugging sessions can leak usable OAuth or API bearer material. Production `info` logs are disabled, so this is not currently a production console leak, but it violates the "never log tokens" rule and is easy to re-enable accidentally.
+
+**Fix:** Remove token value logging entirely. Keep boolean/length-only diagnostics if required, and add a small frontend logger redaction helper for keys containing `token`, `authorization`, `secret`, `password`, or `code`.
+
+### H4 - Generic file delete/info routes are broken and the service path handling is unsafe for a future repair
+
+**Files:** `backend/src/services/file/fileRoutes.ts:71-80`, `backend/src/services/file/fileController.ts:247-280`, `backend/src/services/file/fileService.ts:225-268`
+
+**Status:** Open
+
+`DELETE /api/files/delete` never supplies `req.params.filename`, while the controller requires it. `GET /api/files/info/:filePath` provides `filePath`, but the controller also reads `filename`. These endpoints currently fail validation rather than doing useful work.
+
+The latent security issue is in the service: `deleteFile(filePath)` and `getFileInfo(filePath)` accept arbitrary raw filesystem paths when the input does not start with `/uploads`. A future route-param fix could accidentally expose arbitrary file stat/delete behavior to any authenticated caller.
+
+**Impact:** Broken file-management functionality now; high-risk latent arbitrary file access/delete if the route is repaired by simply passing request params through.
+
+**Fix:** Replace path-based delete/info with file identifiers or strict allow-listed upload-relative paths. Resolve with `path.resolve`, require the final path to stay under the configured upload directory, reject `..`, reject absolute paths, and add route tests for traversal inputs.
+
+### H5 - Template batch endpoints accept unbounded arrays and perform per-id/per-item work
+
+**Files:** `backend/src/services/marketplace/requestTemplateRoutes.ts:267-325`, `backend/src/services/marketplace/RequestTemplateService.ts:472-663`
+
+**Status:** Open
+
+`batch-convert` validates that `items` is an array with `min: 1`, but no maximum. Each item can request `quantity` up to 100 and the service loops items sequentially, creates requests/offers, increments usage, re-fetches populated purchase requests, and schedules notifications.
+
+`complete-payment` validates `requestIds` with `min: 1`, but no maximum, then runs:
+
+```ts
+await Promise.all(requestIds.map((id) => this.marketplaceRepo.findPurchaseRequestById(id)))
+```
+
+before the bulk status update.
+
+**Impact:** An authenticated caller can submit large arrays within the request body limit and fan out many DB reads/writes or a large concurrent read burst. This is a DB/performance and abuse-control issue.
+
+**Fix:** Add explicit max lengths for `items` and `requestIds` (for example 50 or the intended cart maximum), reject duplicate request ids, and move the ownership check to a single repository method that fetches all requested rows by id.
+
+### H6 - User-facing legacy user list exposes broad user data to any authenticated account
+
+**Files:** `backend/src/app.ts:591-592`, `backend/src/services/user/userRoutes.ts:830-879`, `backend/src/services/user/userRoutes.ts:988-1006`
+
+**Status:** Open
+
+The legacy `/api/users` route is still mounted. `GET /api/users` requires authentication but no role restriction, allows `limit` up to 1000, and returns email, role, status, profile fields, and last login metadata for matching active users. The search endpoint also returns email and profile contact metadata to any authenticated caller with a two-character search term.
+
+**Impact:** Broad authenticated user enumeration and privacy exposure. This also creates avoidable DB load because the route still uses Mongoose `User.find`/`countDocuments` rather than the newer repository path.
+
+**Fix:** Decide whether non-admin user discovery is still a product requirement. If not, make `/api/users` admin-only. If yes, narrow returned fields, cap limits much lower, remove email unless explicitly needed, and move it behind a purpose-specific contacts/search endpoint.
+
+## Medium Findings
+
+### M1 - Purchase-request dispute-hold route uses raw ID string comparisons
+
+**Files:** `backend/src/services/dispute/disputeRoutes.ts:44-50`, `backend/src/services/dispute/disputeRoutes.ts:146-153`
+
+**Status:** Open
+
+The `/api/disputes/pr/:purchaseRequestId/...` route compares `request.buyerId.toString()` and preferred seller ids directly against `req.user.id`. In this codebase, many repositories now resolve UUID and legacy ObjectId forms. Raw comparison can false-deny legitimate users when one side is UUID and the other is legacy id.
+
+**Impact:** Compatibility/logic failure, especially during mixed legacy/UUID flows. This looks more like false-deny than privilege bypass.
+
+**Fix:** Reuse the existing `sameUser`/canonical-id helper or expose canonical participant checks from the repository.
+
+### M2 - Dashboard dispute participant checks assume populated `_id` shapes
+
+**Files:** `backend/src/controllers/disputeController.ts:125-130`, `backend/src/controllers/disputeController.ts:253-258`, `backend/src/db/repositories/drizzle/DrizzleDisputeRepo.ts:271-278`
+
+**Status:** Open
+
+The controller checks `dispute.buyerId._id.toString()` and `dispute.sellerId?._id.toString()`. Drizzle dispute records currently map these references into `{ _id: displayId }`, so this works today, but the controller is tightly coupled to a Mongoose-like shape rather than using canonical participant helpers.
+
+**Impact:** Future repository shape changes can break dispute reads/evidence with 500s or false denials.
+
+**Fix:** Add a small `isDisputeParticipant(dispute, user)` helper using `toIdString` plus canonical user comparison, and use it for details/evidence access.
+
+### M3 - Public `/uploads` serving exposes all non-chat uploaded files cross-origin
+
+**Files:** `backend/src/app.ts:527-551`, `backend/src/services/file/fileRoutes.ts:21-69`, `backend/src/services/file/fileService.ts:75-109`
+
+**Status:** Open
+
+Chat attachments are explicitly blocked from static serving, but all other uploads are served through `/uploads` with `Cross-Origin-Resource-Policy: cross-origin`. Generic upload accepts images, PDFs, and Word documents by MIME type and returns a public URL.
+
+**Impact:** This may be intended for avatars, blog images, and product/request-template images, but generic temp/documents uploads become public once uploaded. There is no ownership check, no signed URL, no magic-byte validation for generic documents, and no malware/content scanning.
+
+**Fix:** Split public media uploads from private documents. Keep only intentional public media under static `/uploads`; serve private documents through authenticated file routes with ownership checks. Validate file signatures, not only MIME.
+
+### M4 - Several list endpoints parse user-supplied limits without consistent normalization
+
+**Files:** `backend/src/controllers/disputeController.ts:70-71`, `backend/src/controllers/pointsController.ts:46-78`, `backend/src/services/payment/paymentRoutes.ts:171-174`, `backend/src/services/chat/chatController.ts:301-355`
+
+**Status:** Open
+
+The DB audit fixed many concrete query caps, but the current codebase still has route handlers that call `parseInt(limit)` and pass the result onward without a shared normalizer. Some downstream repos cap values, some do not, and malformed/negative values can produce inconsistent behavior.
+
+**Impact:** Performance variability and route-specific edge cases. This is lower priority than H5 because it requires endpoint-specific downstream confirmation.
+
+**Fix:** Add a shared `parsePagination` helper with min/max/defaults and replace local ad hoc parsing.
+
+## Follow-up Scan Queue
+
+1. Security wave: fix/verify H1-H4 first, then re-scan authz on mounted marketplace/payment routes.
+2. DB/performance wave: quantify H5 with expected max cart size; inspect remaining route-level `limit` parsing and user-route Mongoose paths.
+3. Logic wave: review delivery-code state transitions, dispute creation/resolution side effects, and payment status mutation routes.
+4. Privacy wave: decide intended visibility for `/api/users`, generic uploaded documents, and dispute evidence URLs.
+5. Regression tests: add at least one failing test per high finding before code fixes where practical.
+
+## Already Checked in This Pass
+
+| Area | Result |
+|---|---|
+| Payment callback auth | `paymentCallbackAuth` fails closed when secret is unset and uses timing-safe comparison. |
+| Chat attachment serving | Static `/uploads/chat` is blocked; authenticated chat attachment route checks membership and path traversal. |
+| Dashboard dispute body `buyerId` | Controller ignores body `buyerId` and uses `req.user.id`; the missing check is purchase-request ownership, not body trust. |
+| Legacy marketplace route module | `backend/src/services/marketplace/routes.ts` still contains old route code, but `app.ts` mounts `marketplaceControllerRouter`, not the legacy router. |
+| Taskmaster | `task-master next` reported no available tasks. |