Security Review Checklist

# Security Review Checklist Walk the diff once per category below. For each finding, record file:line, the attack in one sentence, and a concrete fix. "Looks fine" is only a valid conclusion after every section has been walked. ## 1. Injection - Trace every external input (request params, headers, file contents, queue messages, env vars) to its sinks. The dangerous sinks: SQL, shell, HTML templates, `eval`-likes, regex compilation, LDAP/XPath, log lines. - SQL must use parameterized queries or the ORM's bind mechanism. String interpolation into a query is a blocker even if the value "comes from our own enum" — callers change. - Shell: prefer arg arrays over string commands; no user input in a string passed to `sh -c`. Flag `child_process.exec`, `os.system`, backticks. - HTML: framework auto-escaping must not be bypassed (`dangerouslySetInnerHTML`, `| safe`, `v-html`). If it is, demand sanitization with an allowlist library and a justification comment. - Path traversal: any filename derived from input must be resolved and checked against a base directory (`startsWith` after `realpath`), not just stripped of `../`. ## 2. Authorization and authentication - Every new endpoint, query, or mutation: who can call it? Point at the line enforcing that. Absence of an auth check is a finding, not an assumption that middleware covers it. - Object-level checks: fetching by ID from user input must verify the record belongs to the caller (IDOR). `WHERE id = ?` without a tenant/owner clause on user-supplied IDs is a blocker. - Check authorization server-side even when the UI hides the button. Hidden is not forbidden. - State-changing endpoints use POST/PUT/DELETE with CSRF protection, never GET. - Privilege changes (role updates, plan changes) must be logged and must not be settable through mass-assignment of a request body onto a model. ## 3. Secrets - Grep the diff for key-shaped strings: `AKIA`, `-----BEGIN`, `xox[bp]-`, `ghp_`, `sk-`, base64 blobs over 30 chars assigned to names containing key/token/secret/password. - Secrets belong in env/secret managers; flag any default value for a secret in code, including test fixtures with real-looking credentials. - Check logging: do new log lines or error messages include tokens, password fields, or full request bodies? - If a secret was committed and then removed in a later commit of the same branch, it is still compromised — require rotation, not just deletion. ## 4. Unsafe deserialization and parsing - Blockers on untrusted input: `pickle.loads`, `yaml.load` without `SafeLoader`, Java native deserialization, PHP `unserialize`, `Marshal.load`. - JSON parsing is fine; what's done with it isn't always — check for prototype pollution patterns (recursive merge of parsed input into objects) in JS. - XML parsers must have external entity resolution disabled (XXE). - Validate after parsing: schema-validate (types, ranges, allowlisted enum values) before the data reaches logic. Parsing is not validation. ## 5. Transport, storage, dependencies - New external calls use HTTPS with certificate verification on (`verify=False`, `rejectUnauthorized: false` are blockers). - Passwords hashed with bcrypt/scrypt/argon2 — never MD5/SHA-family alone. Comparisons of secrets use constant-time functions. - New dependencies: check for known CVEs and that the package name isn't a typosquat of what was intended. ## Report format One line per finding: `severity | file:line | attack | fix`. Severities: critical (exploitable now), high (exploitable with conditions), medium (defense-in-depth gap), low/info. Close with the sections you walked and found clean, so the reader knows the absence of findings was earned.