5.7 KiB
5.7 KiB
Code Review Example
Scenario
A senior developer is reviewing a junior developer's pull request for a user authentication function. The code works but needs improvement for production readiness.
Input Provided to AI
Code to Review:
def login_user(username, password):
users = [
{"username": "admin", "password": "admin123"},
{"username": "user1", "password": "password"},
{"username": "user2", "password": "test123"}
]
for user in users:
if user["username"] == username and user["password"] == password:
return True
return False
def register_user(username, password):
if len(password) < 6:
return "Password too short"
return "User registered successfully"
Focus Areas: Security, best practices, error handling
AI Output
Code Review Analysis
CRITICAL Security Issues:
-
Plaintext Password Storage (Line 3-7)
- Issue: Passwords stored in plaintext arrays
- Risk: Complete credential exposure if code is compromised
- Fix: Implement proper password hashing with bcrypt/argon2
-
Hardcoded User Database (Line 2-6)
- Issue: User credentials hardcoded in source code
- Risk: Credentials visible in version control
- Fix: Move to secure database with environment-based config
HIGH Priority Issues:
-
No Input Validation (Both functions)
- Issue: No validation for None/empty inputs
- Risk: Runtime errors and potential injection attacks
- Fix: Add comprehensive input sanitization
-
Weak Password Policy (Line 2, register_user)
- Issue: Only checks length, no complexity requirements
- Risk: Vulnerable to brute force attacks
- Fix: Implement strong password validation
MEDIUM Priority Issues:
-
No Rate Limiting
- Issue: Unlimited login attempts possible
- Risk: Brute force vulnerability
- Fix: Add login attempt tracking and temporary lockouts
-
Poor Error Messages (register_user)
- Issue: Generic error messages
- Risk: Poor user experience
- Fix: Provide specific, helpful error descriptions
Recommended Improvements
import bcrypt
import re
from typing import Optional
from datetime import datetime, timedelta
class UserAuthenticator:
def __init__(self, db_connection):
self.db = db_connection
self.failed_attempts = {}
self.lockout_duration = timedelta(minutes=15)
def login_user(self, username: str, password: str) -> dict:
# Input validation
if not username or not password:
return {"success": False, "error": "Username and password required"}
# Check for account lockout
if self._is_account_locked(username):
return {"success": False, "error": "Account temporarily locked"}
# Fetch user from database
user = self.db.get_user_by_username(username)
if not user:
self._record_failed_attempt(username)
return {"success": False, "error": "Invalid credentials"}
# Verify password hash
if bcrypt.checkpw(password.encode('utf-8'), user['password_hash']):
self._clear_failed_attempts(username)
return {"success": True, "user_id": user['id']}
else:
self._record_failed_attempt(username)
return {"success": False, "error": "Invalid credentials"}
def register_user(self, username: str, password: str, email: str) -> dict:
# Comprehensive validation
validation_result = self._validate_registration_data(username, password, email)
if not validation_result["valid"]:
return {"success": False, "errors": validation_result["errors"]}
# Check if user exists
if self.db.user_exists(username, email):
return {"success": False, "error": "User already exists"}
# Hash password and save user
password_hash = bcrypt.hashpw(password.encode('utf-8'), bcrypt.gensalt())
user_id = self.db.create_user(username, password_hash, email)
return {"success": True, "user_id": user_id}
def _validate_registration_data(self, username: str, password: str, email: str) -> dict:
errors = []
if not username or len(username) < 3:
errors.append("Username must be at least 3 characters")
if not self._is_strong_password(password):
errors.append("Password must be at least 8 characters with uppercase, lowercase, number, and special character")
if not re.match(r'^[^@]+@[^@]+\.[^@]+$', email or ''):
errors.append("Valid email address required")
return {"valid": len(errors) == 0, "errors": errors}
Analysis
What Worked Well
- Clear identification of critical security vulnerabilities
- Prioritized issues by severity level
- Provided specific, actionable fixes
- Included improved code example with proper patterns
Areas for Improvement
- Could include more specific examples of attack vectors
- Missing discussion of logging and monitoring requirements
- Could suggest specific testing strategies for the fixes
Key Takeaways
- Security should always be the top priority in code reviews
- Provide constructive, specific feedback with examples
- Explain the "why" behind recommendations
- Offer concrete solutions, not just problem identification
Variations
- Quick Review: Focus only on critical issues for faster turnaround
- Mentoring Style: Include more educational context for junior developers
- Production Focus: Emphasize scalability and monitoring aspects
- Security Audit: Deep dive into all potential security vulnerabilities