code-review-security

What this skill does

# 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:**
```python
# 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:**
```python
# 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:**
```python
# 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:**
```python
# 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:**
```python
# 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