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
12 KiB
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:
// 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:
- Current bid is $100
- User A submits $150 bid
- User B submits $120 bid simultaneously
- Both read currentBid as $100
- Both pass validation
- Whichever writes last wins (could be the $120 bid!)
Fix Solution:
// ✅ 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):
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:
// ✅ 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:
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:
// 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:
// 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:
// 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:
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:
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:
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:
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:
// Anyone can spam requests!
app.post('/api/bids', async (req, res) => {
// No rate limiting
});
Impact:
- DoS vulnerability
- Resource exhaustion
- Abuse potential
Fix Solution:
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:
// 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:
// 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:
app.post('/api/articles', async (req, res) => {
const article = insertArticleSchema.parse(req.body); // Repeated pattern
});
Impact:
- Code duplication
- Maintenance burden
Fix Solution:
// 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:
- ✅ Fix race condition in bidding (P0)
- ✅ Fix HTTP status codes (P0)
- ✅ Add request validation (P0)
- ✅ Add authentication checks (P1)
- ✅ Add input sanitization (P1)
- ⚠️ Add rate limiting (P2)
- ⚠️ 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