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)

A10: Server-Side Request Forgery (SSRF)

What to look for:

• User-supplied URLs used in server-side requests
• Redirect endpoints that accept arbitrary URLs

Python checks:

# BAD: Fetch arbitrary URL from user input
url = request.query_params["url"]
response = httpx.get(url)  # SSRF: can access internal services

# GOOD: Validate URL against allowlist
ALLOWED_HOSTS = {"api.example.com", "cdn.example.com"}
parsed = urlparse(url)
if parsed.hostname not in ALLOWED_HOSTS:
    raise HTTPException(400, "URL not allowed")
response = httpx.get(url)

Review checklist:

• No server-side requests to user-controlled URLs without validation
• URL allowlists used for external integrations
• Internal service URLs not exposed in error messages

Python-Specific Security Checks

Beyond OWASP, review Python code for these patterns:

React-Specific Security Checks

React code review:

// BAD: XSS via dangerouslySetInnerHTML
<div dangerouslySetInnerHTML={{ __html: userBio }} />

// GOOD: Sanitize first, or use text content
import DOMPurify from "dompurify";
<div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(userBio) }} />

// BETTER: Use text content when HTML is not needed
<p>{userBio}</p>

// BAD: javascript: URL
<a href={userLink}>Click</a>  // userLink could be "javascript:alert(1)"

// GOOD: Validate protocol
const safeHref = /^https?:\/\//.test(userLink) ? userLink : "#";
<a href={safeHref}>Click</a>

Severity Classification

Classify each finding by severity for prioritization:

Finding Report Format

When reporting security findings, use this format for consistency:

## Security Finding: [Title]

**Severity:** Critical | High | Medium | Low | Info
**Category:** OWASP A01-A10 or custom category
**File:** path/to/file.py:42
**CWE:** CWE-89 (if applicable)

### Description
Brief description of the vulnerability and its impact.

### Vulnerable Code
```python
# The problematic code
vulnerable_function(user_input)

Recommended Fix

# The secure alternative
safe_function(sanitize(user_input))

Impact

What an attacker could achieve by exploiting this vulnerability.

References

• Link to relevant OWASP page
• Link to relevant CWE entry

### Automated Scanning

Use `scripts/security-scan.py` to perform AST-based scanning for common vulnerability patterns in Python code. The script scans for:
- `eval()` / `exec()` / `compile()` calls
- `subprocess` with `shell=True`
- `pickle.loads()` on potentially untrusted data
- Raw SQL string construction
- `yaml.load()` without `Loader=SafeLoader`
- Hardcoded secret patterns (API keys, passwords)
- Weak hash functions (MD5, SHA1 for passwords)

Run: `python scripts/security-scan.py --path ./app --output-dir ./security-results`

**Dependency scanning (run separately):**
```bash
# Python dependencies
pip-audit --requirement requirements.txt --output json > dep-audit.json

# npm dependencies
npm audit --json > npm-audit.json

Examples

Example Review Comment (Critical)

SECURITY: SQL Injection (Critical, OWASP A03)

File: app/repositories/user_repository.py:47

query = f"SELECT * FROM users WHERE name LIKE '%{search_term}%'"

This constructs a raw SQL query with string interpolation, allowing SQL injection. An attacker could input '; DROP TABLE users; -- to destroy data.

Fix: Use SQLAlchemy ORM filtering:

users = db.query(User).filter(User.name.ilike(f"%{search_term}%")).all()

Example Review Comment (Medium)

SECURITY: Missing Rate Limiting (Medium, OWASP A04)

File: app/routes/auth.py:12

The /auth/login endpoint has no rate limiting. An attacker could perform brute-force password attacks at unlimited speed.

Fix: Add rate limiting middleware:

from slowapi import Limiter
limiter = Limiter(key_func=get_remote_address)

@router.post("/login")
@limiter.limit("5/minute")
async def login(request: Request, ...):

Output File

Write security findings to security-review.md:

# Security Review: [Feature/PR Name]

## Summary
- Critical: 0 | High: 1 | Medium: 2 | Low: 1

## Findings

### [CRITICAL] SQL Injection in user search
- **File:** app/routes/users.py:45
- **OWASP:** A03 Injection
- **Description:** Raw SQL with string interpolation
- **Recommendation:** Use SQLAlchemy ORM filtering

### [HIGH] Missing authorization check
...

## Passed Checks
- No hardcoded secrets found
- Dependencies up to date