Identifies and proposes solutions for XSS, SQL Injection risks, and missing CSRF protection across backend and frontend code. 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
637 lines
15 KiB
Markdown
637 lines
15 KiB
Markdown
# Security Issues - Code Review Report
|
|
|
|
## 🔴 Critical Security Issues
|
|
|
|
### 1. No Input Sanitization - XSS Vulnerability
|
|
**Severity:** CRITICAL (OWASP Top 10 #3)
|
|
**Files Affected:**
|
|
- `server/routes.ts` (all POST/PATCH endpoints)
|
|
- Frontend components rendering user content
|
|
|
|
**Problem:**
|
|
User-supplied content stored and displayed without sanitization:
|
|
```typescript
|
|
// Backend - No sanitization
|
|
app.post('/api/articles', async (req, res) => {
|
|
const article = insertArticleSchema.parse(req.body);
|
|
await storage.createArticle({
|
|
...article,
|
|
content: req.body.content, // ❌ UNSANITIZED HTML/JavaScript!
|
|
title: req.body.title // ❌ UNSANITIZED!
|
|
});
|
|
});
|
|
|
|
// Frontend - Dangerous rendering
|
|
<div dangerouslySetInnerHTML={{ __html: article.content }} />
|
|
```
|
|
|
|
**Attack Vector:**
|
|
```javascript
|
|
// Attacker posts article with malicious content
|
|
{
|
|
title: "Normal Article",
|
|
content: "<script>steal_cookies()</script><img src=x onerror='alert(document.cookie)'>"
|
|
}
|
|
// When other users view this article, the script executes!
|
|
```
|
|
|
|
**Impact:**
|
|
- **Cookie theft** (session hijacking)
|
|
- **Credential theft** (keylogging)
|
|
- **Malware distribution**
|
|
- **Defacement**
|
|
- **Phishing attacks**
|
|
- Affects ALL users viewing malicious content
|
|
|
|
**Affected Data:**
|
|
- Article content & titles
|
|
- Comments
|
|
- Media outlet descriptions
|
|
- User profile data
|
|
- Any user-generated content
|
|
|
|
**Fix Solution:**
|
|
```typescript
|
|
// Backend sanitization
|
|
import DOMPurify from 'isomorphic-dompurify';
|
|
|
|
app.post('/api/articles', async (req, res) => {
|
|
const article = insertArticleSchema.parse(req.body);
|
|
|
|
// ✅ Sanitize all user input
|
|
await storage.createArticle({
|
|
...article,
|
|
title: DOMPurify.sanitize(req.body.title, {
|
|
ALLOWED_TAGS: [] // Strip all HTML from titles
|
|
}),
|
|
content: DOMPurify.sanitize(req.body.content, {
|
|
ALLOWED_TAGS: ['p', 'br', 'b', 'i', 'u', 'a', 'img'],
|
|
ALLOWED_ATTR: ['href', 'src', 'alt'],
|
|
ALLOW_DATA_ATTR: false
|
|
}),
|
|
excerpt: DOMPurify.sanitize(req.body.excerpt, {
|
|
ALLOWED_TAGS: []
|
|
})
|
|
});
|
|
});
|
|
|
|
// Frontend - Use safe rendering
|
|
import DOMPurify from 'dompurify';
|
|
|
|
// ✅ Sanitize before rendering
|
|
<div
|
|
dangerouslySetInnerHTML={{
|
|
__html: DOMPurify.sanitize(article.content)
|
|
}}
|
|
/>
|
|
|
|
// OR better - use React components instead of HTML
|
|
<div>{article.content}</div> // React auto-escapes
|
|
```
|
|
|
|
**Priority:** P0 - IMMEDIATE FIX REQUIRED
|
|
|
|
---
|
|
|
|
### 2. SQL Injection Risk (Potential)
|
|
**Severity:** CRITICAL (OWASP Top 10 #1)
|
|
**Files Affected:** `server/storage.ts`
|
|
|
|
**Problem:**
|
|
While Drizzle ORM prevents most SQL injection, raw SQL queries are vulnerable:
|
|
```typescript
|
|
// ❌ If any raw SQL used with user input
|
|
const results = await db.execute(
|
|
sql`SELECT * FROM articles WHERE slug = ${userInput}` // Potentially unsafe
|
|
);
|
|
```
|
|
|
|
**Impact:**
|
|
- Database breach
|
|
- Data theft
|
|
- Data deletion
|
|
- Privilege escalation
|
|
|
|
**Fix Solution:**
|
|
```typescript
|
|
// ✅ Always use parameterized queries
|
|
const results = await db
|
|
.select()
|
|
.from(articles)
|
|
.where(eq(articles.slug, userInput)); // Drizzle handles escaping
|
|
|
|
// ✅ If raw SQL needed, use prepared statements
|
|
const stmt = db.prepare(
|
|
sql`SELECT * FROM articles WHERE slug = $1`
|
|
);
|
|
const results = await stmt.execute([userInput]);
|
|
```
|
|
|
|
**Priority:** P0 - Audit all queries
|
|
|
|
---
|
|
|
|
### 3. Missing CSRF Protection
|
|
**Severity:** HIGH (OWASP Top 10 #8)
|
|
**Files Affected:**
|
|
- `server/index.ts`
|
|
- All state-changing endpoints
|
|
|
|
**Problem:**
|
|
No CSRF tokens on state-changing operations:
|
|
```typescript
|
|
// ❌ POST/PUT/DELETE endpoints have no CSRF protection
|
|
app.post('/api/articles', async (req, res) => {
|
|
// Attacker can make user create article from malicious site!
|
|
});
|
|
|
|
app.delete('/api/articles/:id', async (req, res) => {
|
|
// Attacker can delete user's articles!
|
|
});
|
|
```
|
|
|
|
**Attack Vector:**
|
|
```html
|
|
<!-- Attacker's website -->
|
|
<form action="https://yourapp.com/api/articles" method="POST">
|
|
<input name="title" value="Spam Article">
|
|
<input name="content" value="Malicious content">
|
|
</form>
|
|
<script>document.forms[0].submit();</script>
|
|
<!-- When logged-in user visits, article created in their name! -->
|
|
```
|
|
|
|
**Impact:**
|
|
- Unauthorized actions
|
|
- Data manipulation
|
|
- Account takeover
|
|
- Financial loss (in bidding system)
|
|
|
|
**Fix Solution:**
|
|
```typescript
|
|
// Install CSRF protection
|
|
import csrf from 'csurf';
|
|
|
|
const csrfProtection = csrf({
|
|
cookie: {
|
|
httpOnly: true,
|
|
secure: process.env.NODE_ENV === 'production',
|
|
sameSite: 'strict'
|
|
}
|
|
});
|
|
|
|
// Apply to all state-changing routes
|
|
app.post('/api/*', csrfProtection);
|
|
app.put('/api/*', csrfProtection);
|
|
app.patch('/api/*', csrfProtection);
|
|
app.delete('/api/*', csrfProtection);
|
|
|
|
// Send token to client
|
|
app.get('/api/csrf-token', csrfProtection, (req, res) => {
|
|
res.json({ csrfToken: req.csrfToken() });
|
|
});
|
|
|
|
// Client must include token
|
|
fetch('/api/articles', {
|
|
method: 'POST',
|
|
headers: {
|
|
'CSRF-Token': csrfToken,
|
|
'Content-Type': 'application/json'
|
|
},
|
|
body: JSON.stringify(data)
|
|
});
|
|
```
|
|
|
|
**Priority:** P1 - Add before production
|
|
|
|
---
|
|
|
|
### 4. Insufficient Authentication Checks
|
|
**Severity:** HIGH
|
|
**Files Affected:** `server/routes.ts`
|
|
|
|
**Problem:**
|
|
Critical endpoints missing authentication:
|
|
```typescript
|
|
// ❌ Anyone can create articles!
|
|
app.post('/api/articles', async (req, res) => {
|
|
// No authentication check!
|
|
});
|
|
|
|
// ❌ Anyone can see all bids!
|
|
app.get('/api/bids', async (req, res) => {
|
|
// No authentication check!
|
|
});
|
|
|
|
// ❌ Anyone can modify media outlets!
|
|
app.patch('/api/media-outlets/:slug', async (req, res) => {
|
|
// Checks inside route, not as middleware
|
|
});
|
|
```
|
|
|
|
**Impact:**
|
|
- Unauthorized data access
|
|
- Spam creation
|
|
- Data manipulation
|
|
- Privilege escalation
|
|
|
|
**Fix Solution:**
|
|
```typescript
|
|
// ✅ Require authentication by default
|
|
const publicRoutes = [
|
|
'/api/media-outlets',
|
|
'/api/articles',
|
|
'/api/auth/user'
|
|
];
|
|
|
|
app.use('/api/*', (req, res, next) => {
|
|
// Allow public routes
|
|
if (publicRoutes.some(route => req.path.startsWith(route))) {
|
|
return next();
|
|
}
|
|
|
|
// Require auth for everything else
|
|
return isAuthenticated(req, res, next);
|
|
});
|
|
|
|
// ✅ Explicit role checks
|
|
const requireAdmin = (req: any, res: Response, next: NextFunction) => {
|
|
if (!req.user || (req.user.role !== 'admin' && req.user.role !== 'superadmin')) {
|
|
return res.status(403).json({ message: "Admin access required" });
|
|
}
|
|
next();
|
|
};
|
|
|
|
app.post('/api/articles', isAuthenticated, requireAdmin, async (req, res) => {
|
|
// Only authenticated admins can create
|
|
});
|
|
```
|
|
|
|
**Priority:** P1 - Fix before production
|
|
|
|
---
|
|
|
|
### 5. Weak Session Configuration
|
|
**Severity:** HIGH
|
|
**Files Affected:**
|
|
- `server/simpleAuth.ts`
|
|
- `server/replitAuth.ts`
|
|
|
|
**Problem:**
|
|
Session cookies lack security flags:
|
|
```typescript
|
|
// ❌ Insecure session config
|
|
app.use(session({
|
|
secret: process.env.SESSION_SECRET || 'dev-secret', // ❌ Weak default
|
|
resave: false,
|
|
saveUninitialized: false,
|
|
cookie: {
|
|
// ❌ Missing security flags!
|
|
maxAge: 30 * 24 * 60 * 60 * 1000 // 30 days - too long!
|
|
}
|
|
}));
|
|
```
|
|
|
|
**Impact:**
|
|
- Session hijacking
|
|
- Cookie theft
|
|
- XSS escalation
|
|
- Man-in-the-middle attacks
|
|
|
|
**Fix Solution:**
|
|
```typescript
|
|
// ✅ Secure session configuration
|
|
app.use(session({
|
|
secret: process.env.SESSION_SECRET, // ✅ Require in production
|
|
resave: false,
|
|
saveUninitialized: false,
|
|
name: 'sessionId', // ✅ Don't reveal framework
|
|
cookie: {
|
|
httpOnly: true, // ✅ Prevent JavaScript access
|
|
secure: process.env.NODE_ENV === 'production', // ✅ HTTPS only in prod
|
|
sameSite: 'strict', // ✅ CSRF protection
|
|
maxAge: 24 * 60 * 60 * 1000, // ✅ 1 day (not 30!)
|
|
domain: process.env.COOKIE_DOMAIN // ✅ Limit to your domain
|
|
},
|
|
store: sessionStore // ✅ Use persistent store (not memory)
|
|
}));
|
|
|
|
// ✅ Validate SESSION_SECRET exists in production
|
|
if (process.env.NODE_ENV === 'production' && !process.env.SESSION_SECRET) {
|
|
throw new Error('SESSION_SECRET must be set in production');
|
|
}
|
|
```
|
|
|
|
**Priority:** P1 - Fix immediately
|
|
|
|
---
|
|
|
|
## 🟠 High Priority Security Issues
|
|
|
|
### 6. No Rate Limiting - DoS Vulnerability
|
|
**Severity:** HIGH
|
|
**Files Affected:** All API endpoints
|
|
|
|
**Problem:**
|
|
No rate limiting allows abuse:
|
|
```typescript
|
|
// ❌ Unlimited requests possible
|
|
app.post('/api/auctions/:id/bid', async (req, res) => {
|
|
// Attacker can spam bids!
|
|
});
|
|
|
|
app.post('/api/comments', async (req, res) => {
|
|
// Attacker can spam comments!
|
|
});
|
|
```
|
|
|
|
**Impact:**
|
|
- Denial of Service
|
|
- Resource exhaustion
|
|
- Bid manipulation
|
|
- Spam flooding
|
|
|
|
**Fix Solution:**
|
|
```typescript
|
|
import rateLimit from 'express-rate-limit';
|
|
|
|
// ✅ General API rate limit
|
|
const apiLimiter = rateLimit({
|
|
windowMs: 15 * 60 * 1000, // 15 minutes
|
|
max: 100, // 100 requests per window
|
|
message: 'Too many requests, please try again later',
|
|
standardHeaders: true,
|
|
legacyHeaders: false,
|
|
});
|
|
|
|
app.use('/api/', apiLimiter);
|
|
|
|
// ✅ Strict limits for sensitive operations
|
|
const bidLimiter = rateLimit({
|
|
windowMs: 60 * 1000, // 1 minute
|
|
max: 5, // 5 bids per minute
|
|
skipSuccessfulRequests: false
|
|
});
|
|
|
|
const commentLimiter = rateLimit({
|
|
windowMs: 60 * 1000,
|
|
max: 10 // 10 comments per minute
|
|
});
|
|
|
|
app.post('/api/auctions/:id/bid', bidLimiter, async (req, res) => {});
|
|
app.post('/api/comments', commentLimiter, async (req, res) => {});
|
|
|
|
// ✅ Login rate limiting (prevent brute force)
|
|
const loginLimiter = rateLimit({
|
|
windowMs: 15 * 60 * 1000,
|
|
max: 5, // 5 login attempts per 15 minutes
|
|
skipSuccessfulRequests: true
|
|
});
|
|
|
|
app.post('/api/login', loginLimiter, async (req, res) => {});
|
|
```
|
|
|
|
**Priority:** P1 - Add before production
|
|
|
|
---
|
|
|
|
### 7. Sensitive Data Exposure in Logs
|
|
**Severity:** MEDIUM
|
|
**Files Affected:** Multiple routes
|
|
|
|
**Problem:**
|
|
Logging sensitive user data:
|
|
```typescript
|
|
catch (error) {
|
|
console.error("Error:", error);
|
|
console.error("Request body:", req.body); // ❌ May contain passwords!
|
|
}
|
|
```
|
|
|
|
**Impact:**
|
|
- Password exposure
|
|
- Session token leaks
|
|
- PII in logs
|
|
|
|
**Fix Solution:**
|
|
```typescript
|
|
// ✅ Sanitize logs
|
|
const sanitizeLog = (data: any) => {
|
|
const sensitive = ['password', 'token', 'secret', 'apiKey'];
|
|
const sanitized = { ...data };
|
|
|
|
for (const key of sensitive) {
|
|
if (key in sanitized) {
|
|
sanitized[key] = '[REDACTED]';
|
|
}
|
|
}
|
|
|
|
return sanitized;
|
|
};
|
|
|
|
catch (error) {
|
|
console.error("Error:", error.message);
|
|
console.error("Request body:", sanitizeLog(req.body));
|
|
}
|
|
```
|
|
|
|
**Priority:** P2 - Implement logging standards
|
|
|
|
---
|
|
|
|
### 8. Missing Security Headers
|
|
**Severity:** MEDIUM
|
|
**Files Affected:** `server/index.ts`
|
|
|
|
**Problem:**
|
|
No security headers configured:
|
|
```typescript
|
|
// ❌ Missing security headers
|
|
app.use(express.json());
|
|
// No helmet, no CSP, no security headers!
|
|
```
|
|
|
|
**Impact:**
|
|
- XSS attacks easier
|
|
- Clickjacking possible
|
|
- MIME sniffing attacks
|
|
- Missing defense-in-depth
|
|
|
|
**Fix Solution:**
|
|
```typescript
|
|
import helmet from 'helmet';
|
|
|
|
// ✅ Add security headers
|
|
app.use(helmet({
|
|
contentSecurityPolicy: {
|
|
directives: {
|
|
defaultSrc: ["'self'"],
|
|
styleSrc: ["'self'", "'unsafe-inline'"],
|
|
scriptSrc: ["'self'"],
|
|
imgSrc: ["'self'", "data:", "https:"],
|
|
connectSrc: ["'self'"],
|
|
fontSrc: ["'self'"],
|
|
objectSrc: ["'none'"],
|
|
mediaSrc: ["'self'"],
|
|
frameSrc: ["'none'"],
|
|
},
|
|
},
|
|
hsts: {
|
|
maxAge: 31536000,
|
|
includeSubDomains: true,
|
|
preload: true
|
|
},
|
|
referrerPolicy: { policy: 'strict-origin-when-cross-origin' }
|
|
}));
|
|
|
|
// ✅ Additional headers
|
|
app.use((req, res, next) => {
|
|
res.setHeader('X-Content-Type-Options', 'nosniff');
|
|
res.setHeader('X-Frame-Options', 'DENY');
|
|
res.setHeader('X-XSS-Protection', '1; mode=block');
|
|
next();
|
|
});
|
|
```
|
|
|
|
**Priority:** P2 - Add security layer
|
|
|
|
---
|
|
|
|
## 🟡 Medium Priority Security Issues
|
|
|
|
### 9. No Content Length Limits
|
|
**Severity:** MEDIUM
|
|
**Files Affected:** `server/index.ts`
|
|
|
|
**Problem:**
|
|
No request size limits:
|
|
```typescript
|
|
app.use(express.json()); // ❌ No size limit!
|
|
```
|
|
|
|
**Impact:**
|
|
- Memory exhaustion
|
|
- DoS via large payloads
|
|
|
|
**Fix Solution:**
|
|
```typescript
|
|
// ✅ Limit request size
|
|
app.use(express.json({ limit: '10mb' })); // Reasonable limit
|
|
app.use(express.urlencoded({ limit: '10mb', extended: true }));
|
|
```
|
|
|
|
**Priority:** P2 - Prevent abuse
|
|
|
|
---
|
|
|
|
### 10. Insecure Direct Object References (IDOR)
|
|
**Severity:** MEDIUM
|
|
**Files Affected:** Various endpoints
|
|
|
|
**Problem:**
|
|
No ownership validation:
|
|
```typescript
|
|
// ❌ User can delete ANY comment by guessing ID!
|
|
app.delete('/api/comments/:id', async (req, res) => {
|
|
await storage.deleteComment(req.params.id);
|
|
// No check if user owns this comment!
|
|
});
|
|
```
|
|
|
|
**Impact:**
|
|
- Unauthorized data access
|
|
- Data deletion
|
|
- Privilege escalation
|
|
|
|
**Fix Solution:**
|
|
```typescript
|
|
// ✅ Validate ownership
|
|
app.delete('/api/comments/:id', isAuthenticated, async (req: any, res) => {
|
|
const comment = await storage.getCommentById(req.params.id);
|
|
|
|
if (!comment) {
|
|
return res.status(404).json({ message: "Comment not found" });
|
|
}
|
|
|
|
// Check ownership or admin
|
|
if (comment.userId !== req.user.id && req.user.role !== 'admin') {
|
|
return res.status(403).json({ message: "Not authorized" });
|
|
}
|
|
|
|
await storage.deleteComment(req.params.id);
|
|
res.json({ success: true });
|
|
});
|
|
```
|
|
|
|
**Priority:** P2 - Add authorization checks
|
|
|
|
---
|
|
|
|
## Security Checklist
|
|
|
|
### Immediate Actions (P0-P1)
|
|
- [ ] Sanitize ALL user input (XSS prevention)
|
|
- [ ] Audit SQL queries for injection risks
|
|
- [ ] Implement CSRF protection
|
|
- [ ] Add authentication to all endpoints
|
|
- [ ] Secure session configuration
|
|
- [ ] Add rate limiting
|
|
- [ ] Review and fix privilege escalation risks
|
|
|
|
### Before Production (P1-P2)
|
|
- [ ] Add security headers (Helmet)
|
|
- [ ] Implement proper logging (no sensitive data)
|
|
- [ ] Add request size limits
|
|
- [ ] Fix IDOR vulnerabilities
|
|
- [ ] Set up security monitoring
|
|
- [ ] Conduct security audit
|
|
- [ ] Penetration testing
|
|
|
|
### Best Practices
|
|
- [ ] Regular security updates
|
|
- [ ] Dependency vulnerability scanning
|
|
- [ ] Security training for team
|
|
- [ ] Incident response plan
|
|
- [ ] Regular backups
|
|
- [ ] Security code review process
|
|
|
|
## Summary
|
|
|
|
| Severity | Count | Must Fix Before Production |
|
|
|----------|-------|----------------------------|
|
|
| 🔴 Critical | 5 | ✅ Yes - BLOCKING |
|
|
| 🟠 High | 3 | ✅ Yes |
|
|
| 🟡 Medium | 2 | ⚠️ Strongly Recommended |
|
|
|
|
**Total Security Issues:** 10
|
|
|
|
**OWASP Top 10 Coverage:**
|
|
- ✅ A01: Broken Access Control (Issues #3, #4, #10)
|
|
- ✅ A02: Cryptographic Failures (Issue #5)
|
|
- ✅ A03: Injection (Issues #1, #2)
|
|
- ✅ A05: Security Misconfiguration (Issues #5, #8)
|
|
- ✅ A07: Identification/Authentication Failures (Issues #4, #5)
|
|
- ✅ A08: Software/Data Integrity Failures (Issue #3)
|
|
|
|
**Estimated Fix Time:**
|
|
- Critical issues: 8-12 hours
|
|
- High priority: 4-6 hours
|
|
- Medium priority: 4-6 hours
|
|
- **Total: 16-24 hours**
|
|
|
|
**Security Hardening Priority:**
|
|
1. Input sanitization (XSS)
|
|
2. CSRF protection
|
|
3. Authentication/authorization
|
|
4. Session security
|
|
5. Rate limiting
|
|
6. Security headers
|
|
7. Audit logging
|
|
8. IDOR fixes
|
|
|
|
**Compliance Notes:**
|
|
- GDPR: Add data encryption, privacy controls
|
|
- PCI DSS: Required if handling payments
|
|
- SOC 2: Implement audit logging
|
|
- HIPAA: Not applicable unless handling health data
|