Addresses race conditions in bid placement using transactions and SELECT FOR UPDATE, and corrects incorrect HTTP status codes for validation errors from 500 to appropriate client error codes (e.g., 400). Replit-Commit-Author: Agent Replit-Commit-Session-Id: aabe2db1-f078-4501-aab5-be145ebc6b9a Replit-Commit-Checkpoint-Type: intermediate_checkpoint Replit-Commit-Screenshot-Url: https://storage.googleapis.com/screenshot-production-us-central1/3df548ff-50ae-432f-9be4-25d34eccc983/aabe2db1-f078-4501-aab5-be145ebc6b9a/TqVS1Ec
520 lines
12 KiB
Markdown
520 lines
12 KiB
Markdown
# 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
|