troykelly/security-review

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 codex-subagent security-reviewer]
**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 (codex-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
• codex-subagent security-reviewer

This skill integrates with:

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

This skill is enforced by:

• .codex/rules/security-sensitive.md conditional rules
• PR gate automation (block PR if security-sensitive without review)