Fix critical security vulnerabilities: secrets, CORS, XSS, input validation

whack-a-mole/SECURITY.md

@@ -0,0 +1,196 @@
+# Security Analysis Report
+
+## Date: 2025-12-31
+
+## Overview
+This document details the security vulnerabilities identified in the Whack-a-Mole game backend and the fixes that have been implemented.
+
+## Security Issues Identified and Fixed
+
+### 1. ⚠️ Hardcoded Secret Key (CRITICAL)
+**Issue:** The Flask application used a hardcoded `SECRET_KEY` value: `"whack-a-mole-secret-key"`
+
+**Risk:**
+- Anyone with access to the source code can see the secret key
+- Attackers can forge session cookies
+- Session hijacking becomes trivial
+- Production and development environments share the same key
+
+**Fix:**
+- Changed to use environment variable: `os.environ.get("SECRET_KEY", os.urandom(24).hex())`
+- Falls back to a cryptographically random key if environment variable not set
+- Production deployments should set the `SECRET_KEY` environment variable
+
+**Recommendation:**
+```bash
+# Set in production environment
+export SECRET_KEY="your-randomly-generated-secret-key-here"
+```
+
+---
+
+### 2. ⚠️ CORS Configuration (HIGH)
+**Issue:** CORS was configured to allow all origins: `cors_allowed_origins="*"`
+
+**Risk:**
+- Any website can make requests to the game server
+- Enables cross-site request forgery (CSRF) attacks
+- No origin validation
+
+**Fix:**
+- Changed to use environment variable for allowed origins
+- Defaults to "*" for development but can be restricted in production
+- Allows comma-separated list of allowed origins
+
+**Recommendation:**
+```bash
+# Set in production environment
+export ALLOWED_ORIGINS="https://yourdomain.com,https://www.yourdomain.com"
+```
+
+---
+
+### 3. ⚠️ XSS Vulnerability in High Scores (HIGH)
+**Issue:** Player names were not sanitized before being stored and displayed
+
+**Risk:**
+- Attackers could inject malicious JavaScript code in player names
+- Cross-site scripting (XSS) attacks
+- Could steal session cookies or perform actions on behalf of other users
+- Example attack: `<script>alert('XSS')</script>` as player name
+
+**Fix:**
+- Implemented `_sanitize_name()` function
+- Uses regex to allow only alphanumeric characters, spaces, hyphens, and underscores
+- Strips all HTML/script tags and special characters
+- Limits name length to 10 characters
+
+---
+
+### 4. ⚠️ Score Validation Missing (MEDIUM)
+**Issue:** Submitted scores were not validated against game state
+
+**Risk:**
+- Players could submit impossibly high scores
+- Score manipulation by modifying client-side requests
+- Unfair leaderboard rankings
+
+**Fix:**
+- Implemented `_validate_score()` function
+- Validates score is an integer
+- Ensures score is non-negative
+- Checks score doesn't exceed maximum possible score based on game duration
+- Rejects invalid submissions
+
+---
+
+### 5. ⚠️ Unsafe Werkzeug in Production (MEDIUM)
+**Issue:** Application ran with `allow_unsafe_werkzeug=True` unconditionally
+
+**Risk:**
+- The development server is not designed for production use
+- Lacks security features of production WSGI servers
+- Vulnerable to DoS attacks
+- Poor performance under load
+
+**Fix:**
+- Made `allow_unsafe_werkzeug` conditional based on `FLASK_DEBUG` environment variable
+- Added warning messages when running in debug mode
+- Recommends using proper WSGI servers (gunicorn, eventlet) in production
+
+**Recommendation:**
+```bash
+# Production deployment
+export FLASK_DEBUG=false
+# Use gunicorn or another production WSGI server
+gunicorn -k eventlet -w 1 backend.app:app
+```
+
+---
+
+### 6. ⚠️ Input Validation in WebSocket Handlers (MEDIUM)
+**Issue:** WebSocket event handlers had minimal input validation
+
+**Risk:**
+- Type confusion attacks
+- Unexpected behavior from malformed inputs
+- Potential crashes from invalid data types
+
+**Fix:**
+- Added strict type checking for all WebSocket inputs
+- Validates `data` parameter is a dict before processing
+- Validates `hole` is an integer within valid range
+- Validates `difficulty` is a string and one of allowed values
+- Returns appropriate error messages for invalid inputs
+
+---
+
+## Additional Security Recommendations
+
+### 7. Rate Limiting
+**Current Status:** Not implemented
+
+**Recommendation:**
+- Implement rate limiting for WebSocket events
+- Prevent spam clicking/cheating by limiting whack attempts per second
+- Limit high score submissions per session
+- Consider using Flask-Limiter or custom rate limiting logic
+
+### 8. Session Management
+**Current Status:** Basic session handling
+
+**Recommendation:**
+- Implement session timeouts
+- Clean up orphaned game states
+- Add maximum concurrent sessions per client
+- Implement reconnection logic with session persistence
+
+### 9. Data Persistence
+**Current Status:** High scores stored in memory (lost on restart)
+
+**Recommendation:**
+- Use a proper database for persistent storage
+- Sanitize data before storage
+- Use parameterized queries to prevent SQL injection
+- Implement backup and recovery mechanisms
+
+### 10. Transport Security
+**Current Status:** HTTP only
+
+**Recommendation:**
+- Use HTTPS in production
+- Enable secure cookies (`SESSION_COOKIE_SECURE=True`)
+- Set `SESSION_COOKIE_HTTPONLY=True`
+- Configure proper SSL/TLS certificates
+
+### 11. Logging and Monitoring
+**Current Status:** Basic print statements
+
+**Recommendation:**
+- Implement structured logging
+- Log security events (failed validations, suspicious activity)
+- Set up monitoring and alerting
+- Use log aggregation tools
+- Never log sensitive data (secrets, passwords)
+
+## Environment Variables Reference
+
+| Variable | Purpose | Default | Production Value |
+|----------|---------|---------|------------------|
+| `SECRET_KEY` | Flask session encryption | Random | Set to strong random value |
+| `ALLOWED_ORIGINS` | CORS allowed origins | `*` | Comma-separated domain list |
+| `FLASK_DEBUG` | Debug mode toggle | `False` | `false` |
+
+## Testing Security Fixes
+
+To test the security improvements:
+
+1. **Secret Key**: Verify sessions are encrypted and unique per deployment
+2. **CORS**: Test that only allowed origins can connect
+3. **XSS**: Attempt to submit names with HTML/script tags
+4. **Score Validation**: Try submitting negative or impossibly high scores
+5. **Input Validation**: Send malformed WebSocket messages
+
+## Conclusion
+
+The implemented security fixes address the most critical vulnerabilities in the application. However, additional hardening is recommended before deploying to production, particularly around rate limiting, HTTPS, and persistent storage security.

whack-a-mole/backend/app.py

@@ -3,7 +3,9 @@
 Multi-threaded Flask server with WebSocket support for real-time game communication.
 """
 
+import os
 import random
+import re
 import threading
 import time
 from dataclasses import dataclass, field
@@ -18,8 +20,12 @@
     static_folder="../frontend/static",
     template_folder="../frontend/templates",
 )
-app.config["SECRET_KEY"] = "whack-a-mole-secret-key"
-socketio = SocketIO(app, cors_allowed_origins="*", async_mode="threading")
+# SECURITY: Use environment variable for secret key instead of hardcoding
+app.config["SECRET_KEY"] = os.environ.get("SECRET_KEY", os.urandom(24).hex())
+# SECURITY: Restrict CORS to specific origins in production
+# For development, you can use "*", but in production set ALLOWED_ORIGINS env var
+allowed_origins = os.environ.get("ALLOWED_ORIGINS", "*").split(",")
+socketio = SocketIO(app, cors_allowed_origins=allowed_origins, async_mode="threading")
 
 
 class Difficulty(Enum):
@@ -307,7 +313,7 @@ def handle_disconnect():
 
 
 @socketio.on("start_game")
-def handle_start_game(data=None):
+def handle_start_game(data=None):  # noqa: PLR0915
     """
     Start a new game with specified difficulty.
 
@@ -353,10 +359,19 @@ def handle_start_game(data=None):
     """
     session_id = request.sid
 
+    # SECURITY: Validate data input
+    if data is not None and not isinstance(data, dict):
+        emit("error", {"message": "Invalid data format"})
+        return
+
     # Parse difficulty from request
     difficulty_str = (data or {}).get("difficulty", "medium")
+    # SECURITY: Validate difficulty is a string and one of allowed values
+    if not isinstance(difficulty_str, str):
+        difficulty_str = "medium"
+
     try:
-        difficulty = Difficulty(difficulty_str)
+        difficulty = Difficulty(difficulty_str.lower())
     except ValueError:
         difficulty = Difficulty.MEDIUM
 
@@ -494,12 +509,18 @@ def handle_whack(data):
     session_id = request.sid
     thread_name = threading.current_thread().name
 
+    # SECURITY: Validate data input
+    if not isinstance(data, dict):
+        emit("whack_result", {"success": False, "error": "Invalid data format"})
+        return
+
     hole = data.get("hole")
     game_state = game_states.get(session_id)
     if not game_state:
         emit("whack_result", {"success": False, "error": "Game not found"})
         return
 
+    # SECURITY: Strict type and range validation for hole number
     if not isinstance(hole, int) or hole < 1 or hole > game_state.max_holes:
         emit("whack_result", {"success": False, "error": "Invalid hole number"})
         return
@@ -551,16 +572,90 @@ def handle_get_high_scores():
     emit("high_scores_update", high_scores)
 
 
+# Maximum multiplier for score validation (conservative upper bound)
+# This accounts for ~2 whacks per second at maximum
+MAX_SCORE_MULTIPLIER = 2
+
+
+def _sanitize_name(name: str) -> str:
+    """
+    Sanitize player name to prevent XSS and injection attacks.
+
+    Args:
+        name: Raw player name input
+
+    Returns:
+        Sanitized name string (alphanumeric, spaces, hyphens, underscores only)
+    """
+    # Remove any HTML/script tags and special characters
+    # Allow only alphanumeric, spaces, hyphens, and underscores
+    sanitized = re.sub(r'[^a-zA-Z0-9\s\-_]', '', str(name))
+    # Limit length and strip whitespace
+    return sanitized[:10].strip() or "Anonymous"
+
+
+def _validate_score(score: any, game_state: GameState) -> bool:
+    """
+    Validate that a submitted score is legitimate.
+
+    Args:
+        score: The score value to validate
+        game_state: Current game state to check bounds
+
+    Returns:
+        True if score is valid, False otherwise
+    """
+    # Score must be an integer
+    if not isinstance(score, int):
+        return False
+    # Score must be non-negative
+    if score < 0:
+        return False
+    # Score should be reasonable (not impossibly high)
+    # Maximum possible score is roughly game_duration / (min mole spawn + processing time)
+    max_possible_score = game_state.game_duration * MAX_SCORE_MULTIPLIER
+    if score > max_possible_score:
+        return False
+    return True
+
+
 @socketio.on("submit_score")
 def handle_submit_score(data):
-    """Handle high score submission."""
-    name = data.get("name", "Anonymous")
+    """
+    Handle high score submission with security validations.
+
+    SECURITY: Validates and sanitizes all user inputs to prevent XSS and
+    injection attacks.
+    """
+    session_id = request.sid
+    game_state = game_states.get(session_id)
+
+    if not game_state:
+        emit("score_submission_error", {"error": "Invalid session"})
+        return
+
+    # SECURITY: Validate data input
+    if not isinstance(data, dict):
+        emit("score_submission_error", {"error": "Invalid data format"})
+        return
+
+    # Sanitize and validate inputs
+    raw_name = data.get("name", "Anonymous")
+    name = _sanitize_name(raw_name)
+
     score = data.get("score", 0)
+    if not _validate_score(score, game_state):
+        emit("score_submission_error", {"error": "Invalid score"})
+        return
+
     difficulty = data.get("difficulty", "medium")
+    # Validate difficulty is one of the allowed values
+    if difficulty not in ["easy", "medium", "hard"]:
+        difficulty = "medium"
 
     # Add new score
     new_entry = {
-        "name": name[:10],  # Limit name length
+        "name": name,
         "score": score,
         "difficulty": difficulty,
         "date": time.strftime("%Y-%m-%d"),
@@ -580,4 +675,22 @@ def handle_submit_score(data):
     print("Whack-a-Mole Game Server")
     print("Multi-threaded backend with WebSocket support")
     print("=" * 50)
-    socketio.run(app, host="0.0.0.0", port=5000, debug=True, allow_unsafe_werkzeug=True)
+
+    # SECURITY: Remove allow_unsafe_werkzeug in production
+    # This flag should only be used in development
+    debug_mode = os.environ.get("FLASK_DEBUG", "False").lower() in ("true", "1")
+
+    if debug_mode:
+        print("WARNING: Running in DEBUG mode with unsafe Werkzeug!")
+        print("         Do NOT use this configuration in production!")
+        socketio.run(
+            app,
+            host="0.0.0.0",
+            port=5000,
+            debug=True,
+            allow_unsafe_werkzeug=True
+        )
+    else:
+        # Production mode - use a proper WSGI server like gunicorn or eventlet
+        print("Running in PRODUCTION mode")
+        socketio.run(app, host="0.0.0.0", port=5000, debug=False)