mcouthon/security-review

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:

1. User inputs - request params, headers, body, URL components
2. Database queries - any SQL/ORM operations
3. Auth/authz checks - where permissions are verified
4. External calls - APIs, services, file system
5. 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:

1. Count direct callers
2. Trace transitive dependencies
3. Identify failure modes
4. Check rollback feasibility
5. 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.