diff --git a/09 - Audits/Security DB Performance Logic Audit - 2026-06-07.md b/09 - Audits/Security DB Performance Logic Audit - 2026-06-07.md new file mode 100644 index 0000000..e75cf05 --- /dev/null +++ b/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. |