Security Review

Overview

Dedicated security review for code handling authentication, authorization, user input, APIs, databases, or credentials.

Core principle: Security issues require specialized attention beyond general code review.

Trigger: This review is MANDATORY when changes touch security-sensitive paths.

Announce at start: "I'm performing a security review of this code."

When Required

This skill is MANDATORY when ANY of these files are modified:

Pattern	Examples
**/auth/**	src/auth/login.ts, lib/auth/session.js
**/security/**	src/security/encryption.ts
**/middleware/**	src/middleware/authenticate.ts
**/api/**	src/api/endpoints.ts
**/*password*	utils/passwordHash.ts
**/*token*	services/tokenService.ts
**/*secret*	config/secrets.ts
**/*credential*	lib/credentials.js
**/*session*	middleware/session.ts
**/routes/**	src/routes/protected.ts
**/*.sql	migrations/001_users.sql

Check with:

git diff --name-only HEAD~1 | grep -E '(auth|security|middleware|api|password|token|secret|credential|session|routes|\.sql)'

OWASP Top 10 Checklist

Review against each category:

1. Injection (A03:2021)

Check	Verify
SQL Injection	All queries use parameterized statements
Command Injection	No user input in shell commands, or properly escaped
LDAP Injection	LDAP queries use proper escaping
XPath Injection	XPath queries use parameterized approach
Template Injection	Template engines configured safely

// VULNERABLE
db.query(`SELECT * FROM users WHERE id = '${userId}'`);

// SECURE
db.query('SELECT * FROM users WHERE id = ?', [userId]);

2. Broken Authentication (A07:2021)

Check	Verify
Password Storage	Passwords hashed with bcrypt/argon2 (not MD5/SHA1)
Session Management	Secure, HttpOnly, SameSite cookies
Token Handling	JWTs signed, validated, short-lived
Brute Force Protection	Rate limiting on auth endpoints
Credential Exposure	No credentials in logs, errors, or responses

3. Sensitive Data Exposure (A02:2021)

Check	Verify
Data in Transit	HTTPS enforced, TLS 1.2+
Data at Rest	Sensitive data encrypted
Secrets in Code	No hardcoded API keys, passwords, tokens
Error Messages	No sensitive info in error responses
Logging	No sensitive data logged

# Check for hardcoded secrets
grep -rE '(password|secret|api_key|token)\s*[:=]\s*["\047][^"\047]+["\047]' src/

4. XML External Entities (A05:2021)

Check	Verify
XML Parsing	External entities disabled
DTD Processing	DTD processing disabled if not needed

5. Broken Access Control (A01:2021)

Check	Verify
Authorization Checks	Every endpoint verifies permissions
Direct Object References	Object access validated against user
Privilege Escalation	Cannot elevate own privileges
CORS	Properly restricted origins
Method Restriction	Only allowed HTTP methods accepted

6. Security Misconfiguration (A05:2021)

Check	Verify
Default Credentials	No default passwords in use
Error Handling	Stack traces not exposed to users
Security Headers	CSP, X-Frame-Options, etc. set
Debug Mode	Disabled in production
Unnecessary Features	Unused endpoints/features removed

7. Cross-Site Scripting (A03:2021)

Check	Verify
Output Encoding	User input encoded before display
DOM XSS	innerHTML not used with user input
Template Safety	Template engine auto-escapes
CSP	Content Security Policy configured

// VULNERABLE
element.innerHTML = userInput;

// SECURE
element.textContent = userInput;
// OR
element.innerHTML = DOMPurify.sanitize(userInput);

8. Insecure Deserialization (A08:2021)

Check	Verify
Object Deserialization	Untrusted data not deserialized
JSON Parsing	Safe JSON.parse usage
Type Validation	Deserialized objects validated

9. Using Components with Known Vulnerabilities (A06:2021)

Check	Verify
Dependency Audit	pnpm audit / pip audit clean
Outdated Packages	No critically outdated dependencies
CVE Check	No known CVEs in dependencies

# Run dependency audit
pnpm audit --prod
# or
pip-audit

10. Insufficient Logging & Monitoring (A09:2021)

Check	Verify
Auth Events	Login success/failure logged
Access Control	Permission denials logged
Input Validation	Validation failures logged
Sensitive Actions	Admin actions logged
Log Integrity	Logs protected from tampering

Review Process

Step 1: Identify Security-Sensitive Changes

# List changed files matching security patterns
git diff --name-only HEAD~1 | grep -E '(auth|security|middleware|api|password|token|secret|session)'

Step 2: Review Each File

For each security-sensitive file:

1. Read the entire file (not just diff)
2. Check against all 10 OWASP categories
3. Note any findings with severity

Step 3: Check Dependencies

# Audit dependencies
pnpm audit --prod

# Check for outdated
pnpm outdated

Step 4: Document Findings

Use severity levels:

Severity	Description	Action
CRITICAL	Exploitable vulnerability, data breach risk	MUST fix before merge
HIGH	Significant security weakness	MUST fix before merge
MEDIUM	Defense-in-depth issue	SHOULD fix before merge
LOW	Minor improvement	MAY fix in future issue

Step 5: Add Security Section to Review Artifact

Add to the main review artifact:

### Security Review

**Security-Sensitive:** YES
**Reviewed By:** [WORKER_ID or security-reviewer subagent]
**OWASP Categories Checked:** 10/10

#### Security Findings

| # | OWASP Category | Severity | Finding | Status |
|---|----------------|----------|---------|--------|
| 1 | A03 Injection | CRITICAL | SQL injection in findUser() | FIXED |
| 2 | A02 Sensitive Data | HIGH | API key in config.ts | DEFERRED #456 |
| 3 | A01 Access Control | MEDIUM | Missing auth on /admin | FIXED |

#### Dependency Audit

pnpm audit: 0 vulnerabilities

**Security Review Status:** [PASS|ISSUES_FIXED|ISSUES_DEFERRED]

Security Review Artifact (Standalone)

If security review is extensive, post as separate comment:

<!-- SECURITY_REVIEW:START -->
## Security Review

| Property | Value |
|----------|-------|
| Issue | #123 |
| Reviewer | `security-reviewer` subagent |
| Reviewed | 2025-12-29T10:30:00Z |

### Files Reviewed

- src/auth/login.ts
- src/middleware/authenticate.ts
- src/api/users.ts

### OWASP Checklist Results

| # | Category | Status | Notes |
|---|----------|--------|-------|
| A01 | Broken Access Control | PASS | Auth middleware on all protected routes |
| A02 | Cryptographic Failures | PASS | bcrypt for passwords, TLS enforced |
| A03 | Injection | FIXED | Parameterized SQL queries now |
| A04 | Insecure Design | PASS | - |
| A05 | Security Misconfiguration | PASS | - |
| A06 | Vulnerable Components | PASS | pnpm audit clean |
| A07 | Auth Failures | PASS | Rate limiting, secure sessions |
| A08 | Data Integrity Failures | PASS | - |
| A09 | Logging Failures | NOTE | Consider adding auth failure logging |
| A10 | SSRF | N/A | No server-side requests |

### Dependency Audit

found 0 vulnerabilities

**Security Review Status:** PASS
<!-- SECURITY_REVIEW:END -->

Checklist

Before completing security review:

• All changed security-sensitive files reviewed
• All 10 OWASP categories checked
• Dependency audit run
• All CRITICAL/HIGH findings fixed
• Any deferred findings have tracking issues
• Security section added to review artifact
• Security-Sensitive marked YES in main artifact

Integration

This skill is triggered by:

• Changes to security-sensitive file patterns
• Explicit invocation
• security-reviewer subagent

This skill integrates with:

• comprehensive-review - Security is criterion #4
• review-gate - Verifies security review for sensitive changes

This skill is enforced by:

• .claude/rules/security-sensitive.md conditional rules
• PreToolUse hook (PR blocked if security-sensitive without review)