mcouthon Security Review
Systematic security analysis of code changes.
Core Approach
"Assume the user is the attacker. Find where trust is misplaced."
Risk Classification
| Risk Level | Triggers |
|---|---|
| HIGH | Auth, crypto, external calls, value transfer, validation removal |
| MEDIUM | Business logic, state changes, new public APIs |
| LOW | Comments, tests, UI, logging |
Attack Surface Mapping
For each change, identify:
- User inputs - request params, headers, body, URL components
- Database queries - any SQL/ORM operations
- Auth/authz checks - where permissions are verified
- External calls - APIs, services, file system
- Cryptographic operations - hashing, encryption, tokens
Security Checklist
Input Validation
- All user input validated before use
- Validation happens at trust boundary (not just client)
- Type coercion handled safely
- Size/length limits enforced
Authentication/Authorization
- Auth checks present on all protected paths
- No privilege escalation paths
- Session handling is secure
- Token expiration enforced
Data Exposure
- No secrets in logs or responses
- Sensitive data filtered from error messages
- PII handling follows policy
- Debug endpoints disabled in production
Injection Prevention
- Parameterized queries for SQL
- Output encoding for XSS
- Command injection prevented
- Path traversal blocked
Cryptography
- No custom crypto implementations
- Strong algorithms used (no MD5/SHA1 for security)
- Secrets not hardcoded
- Key rotation possible
Blast Radius Analysis
For HIGH risk changes:
- Count direct callers
- Trace transitive dependencies
- Identify failure modes
- Check rollback feasibility
- Assess data exposure scope
Red Flags (Stop and Escalate)
- 🔴 Removed validation without replacement
- 🔴 Access control modifiers weakened
- 🔴 External calls added without error handling
- 🔴 Crypto operations changed
- 🔴 Auth bypass paths introduced
- 🔴 Secrets in source code
- 🔴
eval()or dynamic code execution - 🔴 Disabled security controls (even "temporarily")
Common Vulnerability Patterns
| Pattern | Look For |
|---|---|
| IDOR | User-controlled IDs without ownership check |
| Mass Assignment | Binding request body directly to models |
| SSRF | User-controlled URLs in server requests |
| Path Traversal | User input in file paths without sanitization |
| Race Condition | Check-then-use without locking |
| Insecure Deserialization | Deserializing untrusted data |
Output Format
For each finding:
**File**: `path/to/file.py:42`
**Risk**: HIGH | MEDIUM | LOW
**Category**: [Input Validation | Auth | Data Exposure | Injection | Crypto]
**Issue**: [Brief description of what's wrong]
**Evidence**: [Specific code or pattern that demonstrates the issue]
**Recommendation**: [What should be done - without implementing it]
Review Summary Template
## Security Review Summary
**Scope**: [Files/changes reviewed]
**Risk Level**: [Overall: HIGH/MEDIUM/LOW]
### Attack Surface
- Inputs: [list]
- External calls: [list]
- Auth points: [list]
### Findings
| # | Risk | Category | File:Line | Issue |
| --- | ---- | -------- | ---------- | ------------------------ |
| 1 | HIGH | Auth | file.py:42 | Missing permission check |
### Recommendations
1. [Priority-ordered list of fixes]
### Not Reviewed
[Areas that need separate review or were out of scope]
What NOT to Do
- ❌ Fix the issues (identify only)
- ❌ Assume "internal only" means safe
- ❌ Skip test files (they often reveal behavior)
- ❌ Trust comments that say "safe" or "validated elsewhere"
- ❌ Ignore configuration files
The Security Reviewer's Creed
"I'm not here to approve—I'm here to find what's missed."
Trust nothing. Verify everything. Document clearly.