168 lines
5.7 KiB
Markdown
168 lines
5.7 KiB
Markdown
# 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:**
|
|
|
|
```python
|
|
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:**
|
|
|
|
1. **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
|
|
|
|
2. **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:**
|
|
|
|
3. **No Input Validation** (Both functions)
|
|
- **Issue**: No validation for None/empty inputs
|
|
- **Risk**: Runtime errors and potential injection attacks
|
|
- **Fix**: Add comprehensive input sanitization
|
|
|
|
4. **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:**
|
|
|
|
5. **No Rate Limiting**
|
|
- **Issue**: Unlimited login attempts possible
|
|
- **Risk**: Brute force vulnerability
|
|
- **Fix**: Add login attempt tracking and temporary lockouts
|
|
|
|
6. **Poor Error Messages** (register_user)
|
|
- **Issue**: Generic error messages
|
|
- **Risk**: Poor user experience
|
|
- **Fix**: Provide specific, helpful error descriptions
|
|
|
|
### Recommended Improvements
|
|
|
|
```python
|
|
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
|
|
|
|
1. Security should always be the top priority in code reviews
|
|
2. Provide constructive, specific feedback with examples
|
|
3. Explain the "why" behind recommendations
|
|
4. 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
|