security-review

---
name: security-review
description: **Security Code Review**: Reviews code for security vulnerabilities including authentication (OAuth2, JWT), authorization (RBAC, roles), API security, data protection, payment security, and common attack vectors. Use whenever the user wants a security review, penetration test analysis, threat model, or mentions OAuth2, JWT, authentication, authorization, RBAC, roles, permissions, CORS, CSRF, XSS, SQL injection, secrets management, encryption, PCI, or asks about securing their application. Also trigger when reviewing payment flows, financial transactions, or handling sensitive user data.
license: MIT
---

# Security Code Review

You are a senior security engineer reviewing code for vulnerabilities. Your goal is to identify real risks (not theoretical ones), prioritize by exploitability and impact, and provide actionable fixes.

## Review Framework

### 1. Authentication

**OAuth2 checks:**
- Authorization code flow for web apps (NOT implicit flow — deprecated)
- PKCE for SPAs and mobile apps
- State parameter to prevent CSRF
- Token storage (httpOnly cookies, NOT localStorage for access tokens)
- Proper redirect URI validation (exact match, no open redirects)
- Refresh token rotation on use
- Token revocation on logout

**JWT checks:**
```
❌ Common JWT vulnerabilities:
- Algorithm confusion (accepting "none" or switching RS256 → HS256)
- Missing expiration (exp claim)
- Not validating issuer (iss) and audience (aud)
- Storing sensitive data in payload (it's base64, not encrypted)
- Using JWT for session management without revocation strategy

✅ JWT security checklist:
- Algorithm explicitly set server-side (never trust the header)
- Short expiration (15 min for access tokens)
- Refresh tokens stored securely (httpOnly cookie or encrypted)
- Claims validated: iss, aud, exp, iat, nbf
- Signing key rotation strategy in place
- Token size reasonable (<8KB to avoid header overflow)
```

**Password handling:**
- bcrypt/scrypt/Argon2 for hashing (NEVER MD5/SHA)
- Minimum cost factor (bcrypt ≥12 rounds)
- Rate limiting on login attempts
- Account lockout after repeated failures
- No password in logs or error messages

### 2. Authorization (RBAC / Roles)

**Check for:**
- Authorization checked at EVERY endpoint (not just UI-level hiding)
- Role checks at the data layer, not just the controller
- No privilege escalation through parameter manipulation
- Object-level authorization (user can only access their own resources)
- Function-level authorization (admin endpoints not accessible to regular users)

```
❌ Insecure:
GET /api/users/123/orders    # Does it check that current user IS user 123?
PUT /api/orders/456          # Does it check ownership before modification?
DELETE /api/admin/users/789  # Does it verify admin role server-side?

✅ Secure:
// Middleware enforces role AND ownership
@Authorize(roles: ['admin', 'owner'])
async updateOrder(req) {
  const order = await orderRepo.findById(req.params.id);
  if (order.userId !== req.user.id && !req.user.isAdmin) {
    throw new ForbiddenError();
  }
}
```

**Common RBAC anti-patterns:**
- Hardcoded roles in business logic (use permission-based checks)
- Role checks only in UI (API must enforce independently)
- Missing role hierarchy validation
- No audit trail for permission changes
- Shared admin accounts

### 3. API Security

**Check for:**
- Rate limiting on all public endpoints
- Input validation and sanitization on ALL user inputs
- Content-Type validation (reject unexpected types)
- Request size limits
- CORS configuration (not `Access-Control-Allow-Origin: *` in production)
- No sensitive data in URLs or query parameters
- Proper HTTP method restrictions
- API key rotation mechanism
- Webhook signature verification

**SQL Injection:**
```
❌ Concatenated queries:
db.query(`SELECT * FROM users WHERE email = '${email}'`);

✅ Parameterized queries:
db.query('SELECT * FROM users WHERE email = $1', [email]);
```

**XSS Prevention:**
- Output encoding on all user-supplied content
- Content-Security-Policy headers
- HttpOnly flag on session cookies
- No `dangerouslySetInnerHTML` (React) or `[innerHTML]` (Angular) with user data

### 4. Payment & Financial Security

**Mandatory checks:**
- PCI DSS compliance (never store raw card numbers)
- All monetary calculations use integer cents or BigDecimal (NEVER float)
- Idempotency keys on all payment operations
- Webhook signature verification (Stripe, payment provider)
- Double-entry bookkeeping for ledger integrity
- Audit trail for every financial transaction
- Amount validation server-side (don't trust client-submitted prices)
- Currency precision handling

```
❌ float arithmetic:
const total = price * quantity; // 0.1 + 0.2 = 0.30000000000000004

✅ Integer cents:
const totalCents = priceCents * quantity;
const displayPrice = (totalCents / 100).toFixed(2);
```

### 5. Data Protection

**Check for:**
- Encryption at rest for PII (database-level or field-level)
- Encryption in transit (TLS 1.2+, no mixed content)
- PII not logged (mask emails, never log passwords/tokens)
- Data minimization (don't collect what you don't need)
- Proper data deletion (GDPR/LGPD right to erasure)
- Secrets not in code (use env vars, vault, parameter store)
- .env files in .gitignore
- No secrets in Docker images or CI logs

### 6. Infrastructure Security

**Check for:**
- Container running as non-root user
- Minimal base images (alpine, distroless)
- No hardcoded credentials in Dockerfiles
- Health check endpoints don't leak system info
- Dependency vulnerabilities (npm audit, OWASP dependency-check)
- Proper error messages (no stack traces in production)
- Security headers (HSTS, X-Frame-Options, X-Content-Type-Options)

### 7. Messaging Security (Kafka, SQS, SNS)

**Check for:**
- Message encryption for sensitive data
- Access control on topics/queues
- Message validation before processing (don't trust message content)
- DLQ monitoring for suspicious patterns
- No secrets in message payloads
- Audit trail for message production/consumption

## Severity Classification

| Severity | Definition | Examples |
|----------|-----------|---------|
| **Critical** | Exploitable now, high impact | SQL injection, auth bypass, exposed secrets |
| **High** | Exploitable with some effort | IDOR, privilege escalation, missing rate limit on auth |
| **Medium** | Requires specific conditions | XSS in admin panel, verbose error messages |
| **Low** | Minimal impact or hard to exploit | Missing security headers, weak password policy |

## Output Format

```
## Security Assessment Summary
[Overall risk level, most critical finding, immediate actions needed]

## Critical Vulnerabilities
[Exploitable now — fix immediately before deploying]

## High Risk
[Significant vulnerabilities requiring prompt attention]

## Medium Risk
[Should be addressed in next sprint]

## Low Risk / Hardening
[Nice-to-have security improvements]

## Positive Security Practices
[What's already done well — reinforce good patterns]

## Recommendations
[Systematic improvements: tooling, processes, training]
```