diff --git a/backend-issues.md b/backend-issues.md new file mode 100644 index 0000000..ab0d9ec --- /dev/null +++ b/backend-issues.md @@ -0,0 +1,519 @@ +# Backend Issues - Code Review Report + +## 🔴 Critical Issues (Immediate Fix Required) + +### 1. Race Condition in Bid Placement - Data Corruption Risk +**Severity:** CRITICAL +**Files Affected:** +- `server/storage.ts` (placeBid method) +- `server/routes.ts` (POST /api/auctions/:id/bid) + +**Problem:** +Bid validation and database update happen in separate statements without transaction: +```typescript +// storage.ts - placeBid method +async placeBid(auctionId: string, userId: string, amount: number, qualityScore: number) { + const auction = await this.getAuctionById(auctionId); + + // ❌ RACE CONDITION: Another bid can come in here! + if (parseFloat(auction.currentBid || "0") >= amount) { + throw new Error("Bid must be higher than current bid"); + } + + // ❌ Two concurrent bids can both pass validation and overwrite each other + await db.insert(bids).values({ /* ... */ }); + await db.update(auctions).set({ currentBid: amount.toString() }); +} +``` + +**Impact:** +- **Data corruption in live auctions** +- Lower bid can win if timing is right +- Auction integrity compromised +- Financial implications for users +- Business logic violations + +**Reproduction Scenario:** +1. Current bid is $100 +2. User A submits $150 bid +3. User B submits $120 bid simultaneously +4. Both read currentBid as $100 +5. Both pass validation +6. Whichever writes last wins (could be the $120 bid!) + +**Fix Solution:** +```typescript +// ✅ Fixed with transaction and SELECT FOR UPDATE +async placeBid(auctionId: string, userId: string, amount: number, qualityScore: number) { + return await db.transaction(async (tx) => { + // Lock the auction row to prevent concurrent updates + const [auction] = await tx + .select() + .from(auctions) + .where(eq(auctions.id, auctionId)) + .for('update'); // PostgreSQL row-level lock + + if (!auction) { + throw new Error("Auction not found"); + } + + const currentBid = parseFloat(auction.currentBid || "0"); + if (currentBid >= amount) { + throw new Error(`Bid must be higher than current bid of $${currentBid}`); + } + + // Insert bid and update auction atomically + const [newBid] = await tx.insert(bids).values({ + id: crypto.randomUUID(), + auctionId, + userId, + amount: amount.toString(), + qualityScore, + createdAt: new Date() + }).returning(); + + await tx.update(auctions) + .set({ + currentBid: amount.toString(), + qualityScore, + updatedAt: new Date() + }) + .where(eq(auctions.id, auctionId)); + + return newBid; + }); +} +``` + +**Priority:** P0 - Fix immediately before production + +--- + +### 2. Incorrect HTTP Status Codes - Validation Errors Return 500 +**Severity:** CRITICAL +**Files Affected:** +- `server/routes.ts` (multiple endpoints) + +**Problem:** +Zod validation errors and client errors return 500 (Internal Server Error): +```typescript +app.post('/api/articles', async (req, res) => { + try { + const article = insertArticleSchema.parse(req.body); // Can throw ZodError + // ... + } catch (error) { + // ❌ All errors become 500, even validation errors! + res.status(500).json({ message: "Failed to create article" }); + } +}); +``` + +**Impact:** +- **Misleading error responses** +- Client errors (400) appear as server errors (500) +- Difficult to debug for frontend +- Poor API design +- Monitoring alerts on client mistakes +- Can't distinguish real server errors + +**Affected Endpoints:** +- POST /api/articles +- POST /api/auctions/:id/bid +- POST /api/media-outlets/:slug/auction/bids +- POST /api/media-outlet-requests +- PATCH /api/media-outlet-requests/:id +- And more... + +**Fix Solution:** +```typescript +// ✅ Fixed with proper error handling +import { ZodError } from "zod"; + +app.post('/api/articles', async (req, res) => { + try { + const article = insertArticleSchema.parse(req.body); + // ... rest of logic + } catch (error) { + // Handle Zod validation errors + if (error instanceof ZodError) { + return res.status(400).json({ + message: "Invalid request data", + errors: error.errors.map(e => ({ + field: e.path.join('.'), + message: e.message + })) + }); + } + + // Handle known business logic errors + if (error instanceof Error) { + if (error.message.includes("not found")) { + return res.status(404).json({ message: error.message }); + } + if (error.message.includes("permission")) { + return res.status(403).json({ message: error.message }); + } + } + + // Genuine server errors + console.error("Server error:", error); + res.status(500).json({ message: "Internal server error" }); + } +}); +``` + +**Priority:** P0 - Fix immediately + +--- + +### 3. Request Status Updates Without Validation +**Severity:** CRITICAL +**File:** `server/routes.ts` + +**Problem:** +Status updates accept any string and don't verify affected rows: +```typescript +app.patch('/api/media-outlet-requests/:id', async (req, res) => { + const { status } = req.body; // ❌ No validation! + const updated = await storage.updateMediaOutletRequest(req.params.id, { status }); + // ❌ Returns null without checking, client gets 200 with null! + res.json(updated); +}); +``` + +**Impact:** +- Invalid status values accepted +- Non-existent IDs return 200 OK with null +- Domain rules violated +- State machine bypassed + +**Fix Solution:** +```typescript +// Define allowed statuses +const allowedStatuses = ['pending', 'approved', 'rejected'] as const; + +app.patch('/api/media-outlet-requests/:id', isAuthenticated, async (req: any, res) => { + try { + const { status } = req.body; + + // Validate status + if (!allowedStatuses.includes(status)) { + return res.status(400).json({ + message: `Invalid status. Must be one of: ${allowedStatuses.join(', ')}` + }); + } + + // Check permissions + const user = req.user; + if (user.role !== 'admin' && user.role !== 'superadmin') { + return res.status(403).json({ message: "Insufficient permissions" }); + } + + const updated = await storage.updateMediaOutletRequest(req.params.id, { status }); + + // Check if request exists + if (!updated) { + return res.status(404).json({ message: "Request not found" }); + } + + res.json(updated); + } catch (error) { + console.error("Error updating request:", error); + res.status(500).json({ message: "Failed to update request" }); + } +}); +``` + +**Priority:** P0 - Fix immediately + +--- + +## 🟠 High Priority Issues + +### 4. Missing Authentication on Sensitive Endpoints +**Severity:** HIGH +**Files Affected:** `server/routes.ts` + +**Problem:** +Some endpoints should require authentication but don't: +```typescript +// Anyone can create articles! +app.post('/api/articles', async (req, res) => { + // ❌ No isAuthenticated middleware! +}); + +// Anyone can see all prediction bets +app.get('/api/prediction-bets', async (req, res) => { + // ❌ No authentication check! +}); +``` + +**Impact:** +- Unauthorized content creation +- Data exposure +- Spam and abuse potential + +**Fix Solution:** +```typescript +// Add authentication middleware +app.post('/api/articles', isAuthenticated, async (req: any, res) => { + // Now req.user is available +}); + +// Check role for admin actions +app.post('/api/media-outlets', isAuthenticated, async (req: any, res) => { + if (req.user.role !== 'admin' && req.user.role !== 'superadmin') { + return res.status(403).json({ message: "Insufficient permissions" }); + } + // ... +}); +``` + +**Priority:** P1 - Add before production + +--- + +### 5. No Input Sanitization - Potential XSS +**Severity:** HIGH +**Files Affected:** Multiple routes + +**Problem:** +User input stored directly without sanitization: +```typescript +const article = insertArticleSchema.parse(req.body); +await storage.createArticle({ + ...article, + content: req.body.content // ❌ No sanitization! +}); +``` + +**Impact:** +- XSS attacks possible +- Script injection +- Data integrity issues + +**Fix Solution:** +```typescript +import DOMPurify from 'isomorphic-dompurify'; + +const article = insertArticleSchema.parse(req.body); +await storage.createArticle({ + ...article, + content: DOMPurify.sanitize(req.body.content), + title: DOMPurify.sanitize(req.body.title) +}); +``` + +**Priority:** P1 - Fix before production + +--- + +## 🟡 Medium Priority Issues + +### 6. Incomplete Error Logging +**Severity:** MEDIUM +**Files Affected:** Multiple routes + +**Problem:** +Errors logged inconsistently: +```typescript +catch (error) { + console.error("Error:", error); // Minimal context + res.status(500).json({ message: "Failed" }); +} +``` + +**Impact:** +- Difficult debugging +- Lost context +- Can't trace issues + +**Fix Solution:** +```typescript +catch (error) { + console.error("Failed to create article:", { + error: error instanceof Error ? error.message : error, + stack: error instanceof Error ? error.stack : undefined, + userId: req.user?.id, + timestamp: new Date().toISOString(), + body: req.body + }); + res.status(500).json({ message: "Failed to create article" }); +} +``` + +**Priority:** P2 - Improve observability + +--- + +### 7. No Rate Limiting +**Severity:** MEDIUM +**Files Affected:** All routes + +**Problem:** +No rate limiting on any endpoint: +```typescript +// Anyone can spam requests! +app.post('/api/bids', async (req, res) => { + // No rate limiting +}); +``` + +**Impact:** +- DoS vulnerability +- Resource exhaustion +- Abuse potential + +**Fix Solution:** +```typescript +import rateLimit from 'express-rate-limit'; + +const apiLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, // 15 minutes + max: 100, // Limit each IP to 100 requests per window + message: 'Too many requests, please try again later' +}); + +app.use('/api/', apiLimiter); + +// Stricter limits for sensitive operations +const bidLimiter = rateLimit({ + windowMs: 60 * 1000, // 1 minute + max: 5, // 5 bids per minute +}); + +app.post('/api/auctions/:id/bid', bidLimiter, isAuthenticated, async (req, res) => { + // ... +}); +``` + +**Priority:** P2 - Add before production + +--- + +## 🟢 Low Priority Issues + +### 8. Inconsistent Response Formats +**Severity:** LOW +**Files Affected:** Multiple routes + +**Problem:** +Success and error responses have different structures: +```typescript +// Success +res.json(data); + +// Error +res.json({ message: "Error" }); + +// Sometimes +res.json({ error: "Error" }); +``` + +**Impact:** +- Confusing for frontend +- Inconsistent parsing + +**Fix Solution:** +Standardize response format: +```typescript +// Standard success +res.json({ + success: true, + data: result +}); + +// Standard error +res.json({ + success: false, + error: { + message: "...", + code: "ERROR_CODE" + } +}); +``` + +**Priority:** P3 - Polish + +--- + +### 9. Missing Request Validation Middleware +**Severity:** LOW +**Files Affected:** All routes + +**Problem:** +Schema validation repeated in every route: +```typescript +app.post('/api/articles', async (req, res) => { + const article = insertArticleSchema.parse(req.body); // Repeated pattern +}); +``` + +**Impact:** +- Code duplication +- Maintenance burden + +**Fix Solution:** +```typescript +// Create reusable middleware +const validateBody = (schema: z.ZodSchema) => { + return (req: Request, res: Response, next: NextFunction) => { + try { + req.body = schema.parse(req.body); + next(); + } catch (error) { + if (error instanceof ZodError) { + return res.status(400).json({ + message: "Validation failed", + errors: error.errors + }); + } + next(error); + } + }; +}; + +// Use it +app.post('/api/articles', + validateBody(insertArticleSchema), + async (req, res) => { + // req.body is already validated + } +); +``` + +**Priority:** P3 - Refactor + +--- + +## Summary + +| Severity | Count | Must Fix Before Production | +|----------|-------|----------------------------| +| 🔴 Critical | 3 | ✅ Yes - Blocking | +| 🟠 High | 2 | ✅ Yes - Important | +| 🟡 Medium | 2 | ⚠️ Recommended | +| 🟢 Low | 2 | ❌ Nice to have | + +**Total Issues:** 9 + +**Critical Path to Production:** +1. ✅ Fix race condition in bidding (P0) +2. ✅ Fix HTTP status codes (P0) +3. ✅ Add request validation (P0) +4. ✅ Add authentication checks (P1) +5. ✅ Add input sanitization (P1) +6. ⚠️ Add rate limiting (P2) +7. ⚠️ Improve error logging (P2) + +**Estimated Fix Time:** +- Critical issues: 4-6 hours +- High priority: 3-4 hours +- Medium priority: 4-6 hours +- Low priority: 4-6 hours + +**Recommended Testing:** +- Load test bid placement with concurrent requests +- Test all error scenarios for proper status codes +- Verify authentication on all protected routes +- Test input sanitization with XSS payloads