From 5cc11242effd08d3ea8721632e7e7894b6f32bd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Gierwia=C5=82o?= Date: Thu, 20 Nov 2025 23:02:59 +0100 Subject: [PATCH] docs: add comprehensive security audit findings to TODO.md --- docs/TODO.md | 309 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 309 insertions(+) diff --git a/docs/TODO.md b/docs/TODO.md index 2daa07f..7fc240d 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -51,6 +51,315 @@ - `test: improve test cleanup - selective deletion instead of wiping tables` - `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 **Dashboard Page (Post-Login):**