docs: add comprehensive security audit findings to TODO.md
This commit is contained in:
Radosław Gierwiało
309
docs/TODO.md
309
docs/TODO.md
@@ -51,6 +51,315 @@
|
|||||||
- `test: improve test cleanup - selective deletion instead of wiping tables`
|
- `test: improve test cleanup - selective deletion instead of wiping tables`
|
||||||
- `test: fix test isolation by using unique test data per suite`
|
- `test: fix test isolation by using unique test data per suite`
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 🔐 SECURITY AUDIT FINDINGS (2025-11-20)
|
||||||
|
|
||||||
|
**Audit Status:** Comprehensive security audit completed by nodejs-security-auditor
|
||||||
|
**Overall Security Grade:** B+ (pending critical fixes)
|
||||||
|
**Production Readiness:** ❌ NOT READY - 3 critical blockers must be resolved
|
||||||
|
**Ready for Production:** 95% (after fixing critical issues)
|
||||||
|
|
||||||
|
### 🚨 CRITICAL ISSUES (BLOCKERS - Must Fix Immediately)
|
||||||
|
|
||||||
|
**Status:** ❌ All must be resolved before production deployment
|
||||||
|
|
||||||
|
1. **❌ HARDCODED AWS CREDENTIALS IN REPOSITORY**
|
||||||
|
- **Severity:** CRITICAL (10/10)
|
||||||
|
- **CWE:** CWE-798 (Use of Hard-coded Credentials)
|
||||||
|
- **File:** `backend/.env.production` (lines with AWS keys)
|
||||||
|
- **Issue:** Live AWS credentials committed to Git repository
|
||||||
|
- **Impact:**
|
||||||
|
- Unauthorized AWS account access
|
||||||
|
- Financial damage from resource abuse
|
||||||
|
- Email spam/phishing using your domain
|
||||||
|
- Potential data breach
|
||||||
|
- **Required Actions:**
|
||||||
|
```bash
|
||||||
|
# 1. ROTATE CREDENTIALS IMMEDIATELY in AWS Console
|
||||||
|
# 2. Remove from Git history:
|
||||||
|
git rm --cached backend/.env.production backend/.env.development
|
||||||
|
git filter-repo --path backend/.env.production --invert-paths
|
||||||
|
# 3. Use environment variables or Docker Secrets instead
|
||||||
|
# 4. Never commit .env.production files again
|
||||||
|
```
|
||||||
|
- **Location:** Backend deployment configuration
|
||||||
|
- **Priority:** IMMEDIATE - These credentials are compromised
|
||||||
|
|
||||||
|
2. **❌ WEAK PRODUCTION JWT SECRET**
|
||||||
|
- **Severity:** CRITICAL (9/10)
|
||||||
|
- **CWE:** CWE-521 (Weak Password Requirements)
|
||||||
|
- **File:** `backend/.env.production` (JWT_SECRET line)
|
||||||
|
- **Issue:** `JWT_SECRET=production-secret-key-CHANGE-THIS-IN-REAL-PRODUCTION`
|
||||||
|
- **Impact:**
|
||||||
|
- Attackers can forge valid JWT tokens
|
||||||
|
- Complete authentication bypass
|
||||||
|
- Account takeover for any user
|
||||||
|
- **Required Fix:**
|
||||||
|
```bash
|
||||||
|
# Generate cryptographically secure secret (minimum 256 bits)
|
||||||
|
node -e "console.log(require('crypto').randomBytes(64).toString('hex'))"
|
||||||
|
# Use output as JWT_SECRET in production environment (not in Git!)
|
||||||
|
```
|
||||||
|
- **Location:** `backend/src/utils/auth.js:7` (implementation is correct, secret is weak)
|
||||||
|
- **Priority:** IMMEDIATE - Complete auth bypass possible
|
||||||
|
|
||||||
|
3. **❌ DEFAULT DATABASE PASSWORD IN PRODUCTION**
|
||||||
|
- **Severity:** CRITICAL (8/10)
|
||||||
|
- **CWE:** CWE-798 (Use of Hard-coded Credentials)
|
||||||
|
- **File:** `docker-compose.yml:196` (db-prod service)
|
||||||
|
- **Issue:** `POSTGRES_PASSWORD=${POSTGRES_PASSWORD:-spotlightcam123}`
|
||||||
|
- **Impact:**
|
||||||
|
- Database compromise if default is used
|
||||||
|
- Access to all user data (emails, password hashes, messages)
|
||||||
|
- **Required Fix:**
|
||||||
|
```yaml
|
||||||
|
# Remove default fallback, force explicit password:
|
||||||
|
- POSTGRES_PASSWORD=${POSTGRES_PASSWORD:?POSTGRES_PASSWORD must be set}
|
||||||
|
```
|
||||||
|
- **Location:** Docker Compose production database service
|
||||||
|
- **Priority:** IMMEDIATE - Database security
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 🔴 HIGH PRIORITY ISSUES (Fix Before Production)
|
||||||
|
|
||||||
|
**Status:** ⏳ Should be resolved before public launch
|
||||||
|
|
||||||
|
4. **⏳ MISSING HTTPS/TLS CONFIGURATION**
|
||||||
|
- **Severity:** HIGH (8/10)
|
||||||
|
- **Files:** `nginx/conf.d.prod/default.conf`, `docker-compose.yml`
|
||||||
|
- **Issue:** Production nginx only listens on HTTP (port 80), no HTTPS/TLS
|
||||||
|
- **Impact:**
|
||||||
|
- All traffic in plaintext (credentials, tokens, videos)
|
||||||
|
- JWT tokens exposed to network sniffing
|
||||||
|
- Session cookies vulnerable to interception
|
||||||
|
- No MITM protection
|
||||||
|
- **Required Actions:**
|
||||||
|
```bash
|
||||||
|
# 1. Obtain SSL certificate (Let's Encrypt recommended)
|
||||||
|
certbot certonly --webroot -w /var/www/html -d spotlight.cam
|
||||||
|
|
||||||
|
# 2. Update nginx config for HTTPS (see audit report for full config)
|
||||||
|
# 3. Update CORS_ORIGIN to https://spotlight.cam
|
||||||
|
# 4. Update Socket.IO to force secure transports
|
||||||
|
```
|
||||||
|
- **Priority:** Required for production deployment
|
||||||
|
|
||||||
|
5. **⏳ MISSING SECURITY HEADERS IN NGINX**
|
||||||
|
- **Severity:** HIGH (6/10)
|
||||||
|
- **File:** `nginx/conf.d.prod/default.conf`
|
||||||
|
- **Issue:** Production nginx missing critical security headers
|
||||||
|
- **Impact:** XSS, clickjacking, MIME sniffing vulnerabilities
|
||||||
|
- **Required Fix:**
|
||||||
|
```nginx
|
||||||
|
add_header X-Frame-Options "SAMEORIGIN" always;
|
||||||
|
add_header X-Content-Type-Options "nosniff" always;
|
||||||
|
add_header X-XSS-Protection "1; mode=block" always;
|
||||||
|
add_header Referrer-Policy "strict-origin-when-cross-origin" always;
|
||||||
|
add_header Content-Security-Policy "default-src 'self'; ..." always;
|
||||||
|
# HSTS after HTTPS is working:
|
||||||
|
# add_header Strict-Transport-Security "max-age=31536000" always;
|
||||||
|
```
|
||||||
|
- **Priority:** Must fix before launch
|
||||||
|
|
||||||
|
6. **⏳ DEPENDENCY VULNERABILITIES**
|
||||||
|
- **Severity:** HIGH (frontend), MODERATE (backend)
|
||||||
|
- **Tool:** npm audit
|
||||||
|
- **Issues:**
|
||||||
|
- Backend: `js-yaml` (moderate - prototype pollution)
|
||||||
|
- Backend: `cookie` via `csurf` (low - out of bounds chars)
|
||||||
|
- Frontend: `glob` (high - command injection CVE, CLI only)
|
||||||
|
- **Required Actions:**
|
||||||
|
```bash
|
||||||
|
cd backend && npm audit fix
|
||||||
|
cd frontend && npm audit fix
|
||||||
|
# Monitor csurf package (maintenance mode) - consider migration
|
||||||
|
```
|
||||||
|
- **Priority:** Run before launch
|
||||||
|
|
||||||
|
7. **⏳ EXCESSIVE REQUEST SIZE LIMIT**
|
||||||
|
- **Severity:** HIGH (DoS potential)
|
||||||
|
- **File:** `nginx/conf.d.prod/default.conf:13`
|
||||||
|
- **Issue:** `client_max_body_size 500M;` - Way too large for API
|
||||||
|
- **Analysis:** WebRTC is P2P (videos don't go through server), API only needs small payloads
|
||||||
|
- **Required Fix:**
|
||||||
|
```nginx
|
||||||
|
client_max_body_size 10M; # Reduced from 500M
|
||||||
|
location /api { client_max_body_size 1M; }
|
||||||
|
location /socket.io { client_max_body_size 64k; }
|
||||||
|
```
|
||||||
|
- **Priority:** Prevent DoS attacks
|
||||||
|
|
||||||
|
8. **⏳ SOCKET.IO CORS CONFIGURATION**
|
||||||
|
- **Severity:** MEDIUM (6/10)
|
||||||
|
- **File:** `backend/src/socket/index.js:21`
|
||||||
|
- **Issue:** CORS_ORIGIN is comma-separated string, Socket.IO expects array
|
||||||
|
- **Required Fix:**
|
||||||
|
```javascript
|
||||||
|
const allowedOrigins = process.env.CORS_ORIGIN
|
||||||
|
? process.env.CORS_ORIGIN.split(',').map(o => o.trim())
|
||||||
|
: ['http://localhost:8080'];
|
||||||
|
io = new Server(httpServer, { cors: { origin: allowedOrigins } });
|
||||||
|
```
|
||||||
|
- **Priority:** Important for multi-origin deployments
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 🟡 MEDIUM PRIORITY RECOMMENDATIONS (Next Sprint)
|
||||||
|
|
||||||
|
**Status:** 🔄 Implement in security hardening sprint
|
||||||
|
|
||||||
|
9. **🔄 BCRYPT COST FACTOR TOO LOW**
|
||||||
|
- **Severity:** MEDIUM (3/10)
|
||||||
|
- **File:** `backend/src/utils/auth.js:7`
|
||||||
|
- **Issue:** `bcrypt.genSalt(10)` - Cost 10 was standard in 2010, OWASP recommends 12+
|
||||||
|
- **Recommendation:**
|
||||||
|
```javascript
|
||||||
|
const BCRYPT_ROUNDS = parseInt(process.env.BCRYPT_ROUNDS) || 12;
|
||||||
|
const salt = await bcrypt.genSalt(BCRYPT_ROUNDS);
|
||||||
|
```
|
||||||
|
- **Trade-off:** 4x slower hashing (400ms vs 100ms), negligible UX impact
|
||||||
|
- **Priority:** Next security update
|
||||||
|
|
||||||
|
10. **🔄 ACCOUNT LOCKOUT USER ENUMERATION**
|
||||||
|
- **Severity:** MEDIUM (2/10)
|
||||||
|
- **File:** `backend/src/controllers/auth.js:108-134`
|
||||||
|
- **Issue:** Different responses reveal if account exists (401 vs 423)
|
||||||
|
- **Recommendation:** Use same error message for locked/non-existent accounts
|
||||||
|
- **Priority:** Optional - low practical risk
|
||||||
|
|
||||||
|
11. **🔄 WEAK SESSION TIMEOUT**
|
||||||
|
- **Severity:** MEDIUM (4/10)
|
||||||
|
- **File:** `backend/src/utils/auth.js`
|
||||||
|
- **Issue:** 24-hour JWT tokens cannot be revoked, too long for stolen tokens
|
||||||
|
- **Recommendation:** Implement refresh token pattern (15min access + 7day refresh)
|
||||||
|
- **Priority:** Before scaling, acceptable for MVP
|
||||||
|
|
||||||
|
12. **🔄 NO RATE LIMITING ON SOCKET.IO EVENTS**
|
||||||
|
- **Severity:** MEDIUM (4/10)
|
||||||
|
- **File:** `backend/src/socket/index.js`
|
||||||
|
- **Issue:** Socket.IO events (messages, signaling) have no rate limiting
|
||||||
|
- **Recommendation:** Implement per-socket rate limiter (30 messages/minute)
|
||||||
|
- **Priority:** Before public launch to prevent spam
|
||||||
|
|
||||||
|
13. **🔄 MISSING INPUT SANITIZATION ON CHAT MESSAGES**
|
||||||
|
- **Severity:** MEDIUM (4/10)
|
||||||
|
- **File:** `backend/src/socket/index.js:212-268`
|
||||||
|
- **Issue:** Chat message content not sanitized before database save
|
||||||
|
- **Recommendation:**
|
||||||
|
```javascript
|
||||||
|
const sanitizedContent = sanitizeHtml(content).slice(0, 2000);
|
||||||
|
```
|
||||||
|
- **Note:** DOMPurify utility exists in `utils/sanitize.js` but not used for chat
|
||||||
|
- **Priority:** Before launch (XSS prevention)
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 🟢 LOW PRIORITY IMPROVEMENTS (Future Hardening)
|
||||||
|
|
||||||
|
**Status:** 📝 Optional enhancements
|
||||||
|
|
||||||
|
14. **📝 PostgreSQL Connection Security**
|
||||||
|
- **Issue:** Dev database exposes port 5432 to host
|
||||||
|
- **Recommendation:** Restrict to localhost or use pgAdmin in Docker
|
||||||
|
- **Status:** Acceptable for development
|
||||||
|
|
||||||
|
15. **📝 Docker Container Running as Root**
|
||||||
|
- **File:** `backend/Dockerfile.prod`
|
||||||
|
- **Issue:** Container doesn't specify non-root user
|
||||||
|
- **Recommendation:** Add `USER nodejs` directive
|
||||||
|
- **Priority:** Best practice, limited impact in containers
|
||||||
|
|
||||||
|
16. **📝 Missing Security Logging**
|
||||||
|
- **Issue:** Security events not systematically logged (login failures, lockouts, token failures)
|
||||||
|
- **Recommendation:** Implement structured security logging
|
||||||
|
- **Priority:** Useful for monitoring, not blocking
|
||||||
|
|
||||||
|
17. **📝 Token Verification Timing Attack**
|
||||||
|
- **Severity:** LOW (2/10)
|
||||||
|
- **File:** `backend/src/controllers/auth.js:305-309`
|
||||||
|
- **Issue:** 6-digit verification code comparison not timing-safe
|
||||||
|
- **Note:** `timingSafeEqual` utility exists, just not used here
|
||||||
|
- **Priority:** Optional - codes expire in 24h with rate limiting
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### ✅ POSITIVE SECURITY FINDINGS
|
||||||
|
|
||||||
|
**Excellent practices already implemented:**
|
||||||
|
|
||||||
|
1. ✅ **Strong Authentication:** JWT, bcrypt, email verification, password reset
|
||||||
|
2. ✅ **Comprehensive Input Validation:** express-validator throughout
|
||||||
|
3. ✅ **Security Headers:** Helmet.js with CSP, XSS protection, HSTS, noSniff
|
||||||
|
4. ✅ **Rate Limiting:** Multiple limiters (API, auth, email)
|
||||||
|
5. ✅ **CORS Configuration:** Proper origin validation
|
||||||
|
6. ✅ **Authorization:** Socket.IO auth middleware, WebRTC signaling authorization
|
||||||
|
7. ✅ **Database Security:** Prisma ORM prevents SQL injection
|
||||||
|
8. ✅ **Error Handling:** Generic messages in production, no stack traces
|
||||||
|
9. ✅ **Account Lockout:** Implemented after failed attempts
|
||||||
|
10. ✅ **Defense in Depth:** Multiple security layers
|
||||||
|
11. ✅ **WebRTC P2P:** Videos don't touch server (privacy + security)
|
||||||
|
12. ✅ **Test Coverage:** 223/223 tests passing (71% coverage)
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 📋 SECURITY DEPLOYMENT CHECKLIST
|
||||||
|
|
||||||
|
**Pre-Deployment (MUST COMPLETE):**
|
||||||
|
- [ ] **CRITICAL:** Rotate AWS credentials and remove from Git history
|
||||||
|
- [ ] **CRITICAL:** Generate strong JWT_SECRET (64+ bytes)
|
||||||
|
- [ ] **CRITICAL:** Set strong PostgreSQL password (no fallback)
|
||||||
|
- [ ] **CRITICAL:** Remove .env files from Git tracking
|
||||||
|
- [ ] **HIGH:** Configure HTTPS/TLS with valid certificate
|
||||||
|
- [ ] **HIGH:** Add nginx security headers
|
||||||
|
- [ ] **HIGH:** Fix npm audit vulnerabilities
|
||||||
|
- [ ] **HIGH:** Reduce nginx body size limit (500M → 10M)
|
||||||
|
- [ ] **MEDIUM:** Update Socket.IO CORS to handle array
|
||||||
|
- [ ] **MEDIUM:** Implement Socket.IO rate limiting
|
||||||
|
|
||||||
|
**Post-Deployment (First 30 Days):**
|
||||||
|
- [ ] Monitor security logs for unusual patterns
|
||||||
|
- [ ] Test account lockout mechanism
|
||||||
|
- [ ] Verify HTTPS enforcement
|
||||||
|
- [ ] Test CSRF protection
|
||||||
|
- [ ] Review and rotate credentials on schedule
|
||||||
|
|
||||||
|
**Ongoing Maintenance:**
|
||||||
|
- [ ] Monthly: npm audit and dependency updates
|
||||||
|
- [ ] Quarterly: Rotate JWT_SECRET (requires re-auth)
|
||||||
|
- [ ] Yearly: Security penetration testing
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 📊 RISK ASSESSMENT SUMMARY
|
||||||
|
|
||||||
|
| Vulnerability | Likelihood | Impact | Risk Score | Status |
|
||||||
|
|---------------|------------|--------|------------|--------|
|
||||||
|
| AWS Credentials Exposed | VERY HIGH | CRITICAL | **10/10** | ❌ MUST FIX |
|
||||||
|
| Weak JWT Secret | HIGH | CRITICAL | **9/10** | ❌ MUST FIX |
|
||||||
|
| Default DB Password | MEDIUM | CRITICAL | **8/10** | ❌ MUST FIX |
|
||||||
|
| Missing HTTPS | HIGH | HIGH | **8/10** | ⏳ MUST FIX |
|
||||||
|
| Missing nginx Headers | MEDIUM | HIGH | **6/10** | ⏳ Should Fix |
|
||||||
|
| Excessive Body Size | LOW | MEDIUM | **4/10** | ⏳ Should Fix |
|
||||||
|
|
||||||
|
**Overall Assessment:** Application is **95% production-ready** from security perspective. The remaining 5% are critical blockers requiring 1-2 days to fix.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 🔗 FULL AUDIT REPORT
|
||||||
|
|
||||||
|
For complete details including:
|
||||||
|
- Specific code examples for each fix
|
||||||
|
- Configuration templates
|
||||||
|
- OWASP Top 10 coverage analysis
|
||||||
|
- Compliance notes (GDPR)
|
||||||
|
- Detailed architecture recommendations
|
||||||
|
|
||||||
|
See the complete security audit report generated by nodejs-security-auditor agent (available in conversation history).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
### 📋 FUTURE UX/UI IMPROVEMENTS
|
### 📋 FUTURE UX/UI IMPROVEMENTS
|
||||||
|
|
||||||
**Dashboard Page (Post-Login):**
|
**Dashboard Page (Post-Login):**
|
||||||
|
|||||||
Reference in New Issue
Block a user