Security Code Review

Identify security vulnerabilities, potential attack vectors, and insecure coding patterns during code review.

When to Use This Skill

• Reviewing code changes for security implications
• Performing security audits on codebases
• Checking for OWASP Top 10 vulnerabilities
• Validating authentication and authorization logic
• Ensuring sensitive data is handled properly

Security Review Workflow

┌────────────────────────────────────────────────────────────┐
│                 SECURITY REVIEW PROCESS                     │
├────────────────────────────────────────────────────────────┤
│  1. Identify attack surface (inputs, APIs, data flow)      │
│  2. Check for injection vulnerabilities                     │
│  3. Validate authentication/authorization                   │
│  4. Review sensitive data handling                          │
│  5. Check cryptographic usage                               │
│  6. Verify error handling doesn't leak info                 │
│  7. Review dependencies for known vulnerabilities           │
└────────────────────────────────────────────────────────────┘

OWASP Top 10 Checklist

A01: Broken Access Control

# ❌ VULNERABLE: No authorization check
@app.get("/users/{user_id}/profile")
def get_profile(user_id: int):
    return db.get_user(user_id)

# ✅ SECURE: Verify user can access resource
@app.get("/users/{user_id}/profile")
def get_profile(user_id: int, current_user: User = Depends(get_current_user)):
    if current_user.id != user_id and not current_user.is_admin:
        raise HTTPException(status_code=403, detail="Access denied")
    return db.get_user(user_id)

Review Checklist:

• All endpoints have authorization checks
• Users can only access their own resources
• Admin functions require admin role
• CORS configured properly
• JWT tokens validated on every request

A02: Cryptographic Failures

# ❌ VULNERABLE: Weak hashing
import hashlib
password_hash = hashlib.md5(password.encode()).hexdigest()

# ❌ VULNERABLE: Hardcoded secret
SECRET_KEY = "my-secret-key-123"

# ✅ SECURE: Proper password hashing
from passlib.context import CryptContext
pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto")
password_hash = pwd_context.hash(password)

# ✅ SECURE: Environment-based secrets
import os
SECRET_KEY = os.environ["SECRET_KEY"]

Review Checklist:

• Passwords hashed with bcrypt/argon2/scrypt
• No MD5/SHA1 for security purposes
• Secrets loaded from environment
• TLS enforced for sensitive data
• Proper key management

A03: Injection

# ❌ VULNERABLE: SQL injection
query = f"SELECT * FROM users WHERE name = '{user_input}'"
cursor.execute(query)

# ❌ VULNERABLE: Command injection
os.system(f"ping {user_input}")

# ✅ SECURE: Parameterized queries
cursor.execute("SELECT * FROM users WHERE name = ?", (user_input,))

# ✅ SECURE: Use safe APIs
import subprocess
subprocess.run(["ping", "-c", "1", validated_host], check=True)

Review Checklist:

• All SQL uses parameterized queries
• No string interpolation in queries
• Command execution uses subprocess with lists
• Template engines auto-escape by default
• LDAP queries use proper escaping

A04: Insecure Design

Review Checklist:

• Rate limiting implemented
• Account lockout after failed attempts
• CAPTCHA for sensitive operations
• Business logic validates constraints
• Fail-secure defaults

A05: Security Misconfiguration

# ❌ VULNERABLE: Debug mode in production
app = Flask(__name__)
app.run(debug=True)

# ❌ VULNERABLE: Default credentials
DATABASE_PASSWORD = "admin"

# ✅ SECURE: Environment-based configuration
app.run(debug=os.environ.get("DEBUG", "false").lower() == "true")

Review Checklist:

• Debug mode disabled in production
• Default credentials changed
• Unnecessary features disabled
• Security headers configured
• Error messages don't reveal internals

A06: Vulnerable Components

# Check for known vulnerabilities
pip-audit
safety check

Review Checklist:

• Dependencies scanned for vulnerabilities
• Outdated packages updated
• Unused dependencies removed
• Lock file maintained

A07: Authentication Failures

# ❌ VULNERABLE: Weak session
session_id = str(random.randint(1000, 9999))

# ❌ VULNERABLE: No brute force protection
def login(username, password):
    user = db.get_user(username)
    return user.password == password

# ✅ SECURE: Strong session tokens
import secrets
session_id = secrets.token_urlsafe(32)

# ✅ SECURE: Rate limiting and constant-time comparison
from slowapi import Limiter
import hmac

@limiter.limit("5/minute")
def login(username, password):
    user = db.get_user(username)
    if not user:
        # Prevent timing attacks
        pwd_context.dummy_verify()
        return False
    return pwd_context.verify(password, user.password_hash)

Review Checklist:

• Strong session tokens (32+ bytes)
• Session timeout implemented
• Password requirements enforced
• Multi-factor authentication available
• Brute force protection

A08: Data Integrity Failures

Review Checklist:

• CI/CD pipeline secured
• Dependencies verified (checksums)
• Serialization validated
• Critical data signed

A09: Logging Failures

# ❌ VULNERABLE: Logging sensitive data
logger.info(f"User login: {username}, password: {password}")

# ❌ VULNERABLE: No audit logging
def delete_user(user_id):
    db.delete(user_id)

# ✅ SECURE: Sanitized logging
logger.info(f"User login attempt: {username}")

# ✅ SECURE: Audit trail
def delete_user(user_id, deleted_by: User):
    audit_log.info(
        "User deleted",
        extra={"user_id": user_id, "deleted_by": deleted_by.id, "timestamp": now()}
    )
    db.delete(user_id)

Review Checklist:

• No passwords/tokens in logs
• Security events logged
• Logs have timestamps
• Log injection prevented
• Logs stored securely

A10: Server-Side Request Forgery (SSRF)

# ❌ VULNERABLE: Unvalidated URL
def fetch_url(url):
    return requests.get(url).content

# ✅ SECURE: URL validation
from urllib.parse import urlparse

ALLOWED_HOSTS = {"api.trusted.com", "cdn.example.com"}

def fetch_url(url):
    parsed = urlparse(url)
    if parsed.hostname not in ALLOWED_HOSTS:
        raise ValueError("Host not allowed")
    if parsed.scheme not in ("http", "https"):
        raise ValueError("Invalid scheme")
    return requests.get(url).content

Review Checklist:

• URLs validated against allowlist
• Internal network access blocked
• Redirect following limited
• Response type validated

Python-Specific Security Issues

Path Traversal

# ❌ VULNERABLE
def read_file(filename):
    with open(f"/uploads/{filename}") as f:
        return f.read()

# ✅ SECURE
from pathlib import Path

UPLOAD_DIR = Path("/uploads").resolve()

def read_file(filename):
    file_path = (UPLOAD_DIR / filename).resolve()
    if not file_path.is_relative_to(UPLOAD_DIR):
        raise ValueError("Invalid path")
    return file_path.read_text()

Pickle Deserialization

# ❌ VULNERABLE: Never unpickle untrusted data
import pickle
data = pickle.loads(user_input)

# ✅ SECURE: Use safe formats
import json
data = json.loads(user_input)

YAML Loading

# ❌ VULNERABLE: Arbitrary code execution
import yaml
data = yaml.load(user_input)

# ✅ SECURE: Safe loader
data = yaml.safe_load(user_input)

Regular Expression DoS (ReDoS)

# ❌ VULNERABLE: Catastrophic backtracking
import re
pattern = re.compile(r"(a+)+$")
pattern.match("a" * 30 + "!")  # Takes forever

# ✅ SECURE: Use atomic groups or limit input
import re
MAX_INPUT_LENGTH = 1000
if len(user_input) > MAX_INPUT_LENGTH:
    raise ValueError("Input too long")

Security Review Comment Templates

Critical Vulnerability

🔴 **CRITICAL SECURITY ISSUE**

**Vulnerability**: [Type, e.g., SQL Injection]
**Location**: [File:Line]
**Risk**: [Impact description]

**Current Code**:
```python
# vulnerable code

Recommended Fix:

# secure code

References:

• [OWASP Link]

### Security Warning

```markdown
⚠️ **SECURITY WARNING**

**Issue**: [Description]
**Risk Level**: Medium

**Recommendation**: [Fix description]

Security Suggestion

💡 **Security Improvement**

Consider implementing [security measure] to enhance defense-in-depth.

Automated Security Checks

# Static analysis
bandit -r src/

# Dependency vulnerabilities
pip-audit
safety check

# Secret detection
detect-secrets scan

# Type checking (catches some security issues)
mypy src/

Resources

• OWASP Top 10
• OWASP Cheat Sheet Series
• Bandit Security Linter
• CWE (Common Weakness Enumeration)

Guidelines

• Assume all input is malicious
• Apply defense in depth (multiple layers)
• Fail securely (deny by default)
• Minimize attack surface
• Keep secrets out of code
• Log security events, not sensitive data
• Update dependencies regularly
• Use established security libraries, don't roll your own crypto