troykelly 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:
- Read the entire file (not just diff)
- Check against all 10 OWASP categories
- 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 #4review-gate- Verifies security review for sensitive changes
This skill is enforced by:
.codex/rules/security-sensitive.mdconditional rules- PR gate automation (block PR if security-sensitive without review)