Code Review Security

Code Review Security

When to Use

Activate this skill when:

• Reviewing pull requests for security vulnerabilities
• Auditing authentication or authorization code changes
• Reviewing code that handles user input, file uploads, or external data
• Checking for OWASP Top 10 vulnerabilities in new features
• Validating that secrets are not committed to the repository
• Scanning dependencies for known vulnerabilities
• Reviewing API endpoints that expose sensitive data

Output: Write findings to security-review.md with severity, file:line, description, and recommendations.

Do NOT use this skill for:

• Deployment infrastructure security (use docker-best-practices)
• Incident response procedures (use incident-response)
• General code quality review without security focus (use pre-merge-checklist)
• Writing implementation code (use python-backend-expert or react-frontend-expert)

Instructions

OWASP Top 10 Checklist

Review every PR against the OWASP Top 10 (2021 edition). Each category below includes specific checks for Python/FastAPI and React codebases.

A01: Broken Access Control

What to look for:

• Missing authorization checks on endpoints
• Direct object reference without ownership verification
• Endpoints that expose data without role-based filtering
• Missing Depends() for auth on new routes

Python/FastAPI checks:

# BAD: No authorization check -- any authenticated user can access any user
@router.get("/users/{user_id}")
async def get_user(user_id: int, db: Session = Depends(get_db)):
    return await user_repo.get(user_id)

# GOOD: Verify the requesting user owns the resource or is admin
@router.get("/users/{user_id}")
async def get_user(
    user_id: int,
    current_user: User = Depends(get_current_user),
    db: Session = Depends(get_db),
):
    if current_user.id != user_id and current_user.role != "admin":
        raise HTTPException(status_code=403, detail="Forbidden")
    return await user_repo.get(user_id)

Review checklist:

• Every route has authentication (Depends(get_current_user))
• Resource access is verified against the requesting user
• Admin-only endpoints check role == "admin"
• List endpoints filter by user ownership (unless admin)
• No IDOR (Insecure Direct Object Reference) vulnerabilities

A02: Cryptographic Failures

What to look for:

• Passwords stored in plaintext or with weak hashing
• Sensitive data in logs or error messages
• Hardcoded secrets, API keys, or tokens
• Weak JWT configuration

Python checks:

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

# GOOD: Use bcrypt via passlib
from passlib.context import CryptContext
pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto")
password_hash = pwd_context.hash(password)

# BAD: Secret in code
SECRET_KEY = "my-super-secret-key-123"

# GOOD: Secret from environment
SECRET_KEY = os.environ["SECRET_KEY"]

Review checklist:

• Passwords hashed with bcrypt (never MD5, SHA1, or plaintext)
• JWT secret loaded from environment, not hardcoded
• Sensitive data excluded from logs (passwords, tokens, PII)
• HTTPS enforced for all external communication
• No secrets in source code (check .env.example has placeholders only)

A03: Injection

What to look for:

• Raw SQL queries with string interpolation
• eval(), exec(), compile() with user input
• subprocess calls with shell=True
• Template injection

Python checks:

# BAD: SQL injection via string formatting
query = f"SELECT * FROM users WHERE email = '{email}'"
db.execute(text(query))

# GOOD: Parameterized query
db.execute(text("SELECT * FROM users WHERE email = :email"), {"email": email})

# GOOD: SQLAlchemy ORM (always parameterized)
user = db.query(User).filter(User.email == email).first()

# BAD: Command injection
subprocess.run(f"convert {filename}", shell=True)

# GOOD: Pass arguments as a list
subprocess.run(["convert", filename], shell=False)

# BAD: Code execution with user input
result = eval(user_input)

# GOOD: Never eval user input. Use ast.literal_eval for safe parsing.
result = ast.literal_eval(user_input)  # Only for literal structures

Review checklist:

• No raw SQL with string interpolation (use ORM or parameterized queries)
• No eval(), exec(), or compile() with external input
• No subprocess.run(..., shell=True) with dynamic arguments
• No pickle.loads() on untrusted data
• All user input validated by Pydantic schemas before use

A04: Insecure Design

What to look for:

• Missing rate limiting on authentication endpoints
• No account lockout after failed login attempts
• Missing CAPTCHA on public-facing forms
• Business logic flaws (e.g., negative amounts, self-privilege-escalation)

Review checklist:

• Rate limiting on login, registration, and password reset
• Account lockout or exponential backoff after 5+ failed attempts
• Business logic validates constraints (positive amounts, valid transitions)
• Sensitive operations require re-authentication

A05: Security Misconfiguration

What to look for:

• Debug mode enabled in production
• CORS configured with wildcard * origins
• Default credentials or admin accounts
• Verbose error messages exposing stack traces

Python/FastAPI checks:

# BAD: Wide-open CORS
app.add_middleware(CORSMiddleware, allow_origins=["*"])

# GOOD: Explicit allowed origins
app.add_middleware(
    CORSMiddleware,
    allow_origins=["https://app.example.com"],
    allow_methods=["GET", "POST", "PUT", "DELETE"],
    allow_headers=["Authorization", "Content-Type"],
)

# BAD: Debug mode in production
app = FastAPI(debug=True)

# GOOD: Debug only in development
app = FastAPI(debug=settings.DEBUG)  # DEBUG=False in production

Review checklist:

• CORS origins are explicit (no wildcard in production)
• Debug mode disabled in production configuration
• Error responses do not expose stack traces or internal details
• Default admin credentials are changed or removed
• Security headers set (X-Content-Type-Options, X-Frame-Options, etc.)

A06: Vulnerable and Outdated Components

Review checklist:

• No known CVEs in Python dependencies (pip-audit or safety check)
• No known CVEs in npm dependencies (npm audit)
• Dependencies pinned to specific versions in lock files
• No deprecated packages still in use

A07: Identification and Authentication Failures

What to look for:

• Weak password policies
• Session tokens that do not expire
• Missing multi-factor authentication for admin actions
• JWT tokens without expiration

Python checks:

# BAD: JWT without expiration
token = jwt.encode({"sub": user_id}, SECRET_KEY, algorithm="HS256")

# GOOD: JWT with expiration
token = jwt.encode(
    {"sub": user_id, "exp": datetime.utcnow() + timedelta(minutes=30)},
    SECRET_KEY,
    algorithm="HS256",
)

Review checklist:

• JWT tokens have expiration (exp claim)
• Refresh tokens are stored securely and can be revoked
• Password policy enforces minimum length (12+) and complexity
• Session invalidation on password change or logout
• No user enumeration via login error messages

A08: Software and Data Integrity Failures

Review checklist:

• CI/CD pipeline validates artifact integrity
• No unsigned or unverified packages
• Deserialization of untrusted data uses safe methods (no pickle.loads)
• Database migrations are reviewed before execution

A09: Security Logging and Monitoring Failures

Review checklist:

• Authentication events are logged (login, logout, failed attempts)
• Authorization failures are logged with context
• Sensitive data is NOT included in logs (passwords, tokens, PII)
• Log entries include timestamp, user ID, IP address, action
• Alerting configured for suspicious patterns (brute force, unusual access)