secure-code-review

When to use

• Reviewing a PR or diff before merge, especially one touching auth, user input, DB/queries, file I/O, outbound requests, or crypto.
• The user asks "any security issues here?" or wants a second pass focused only on vulnerabilities (not style).
• Complements secscan (automated) — this is the human-grade reasoning pass.

Process

1. Get the diff scope: git diff main...HEAD or the PR's changed files. Review the change, not the whole repo, but read enough surrounding code to judge data flow.
2. Trace untrusted input from entry point to sink — where does external data reach a query, command, file path, template, or response?
3. Walk the checklist below against the changed code.
4. For each finding, assign a severity (Critical/High/Medium/Low) and write the exact file:line, the risk, and a concrete fix.
5. Distinguish real exploitable issues from theoretical ones; don't drown the author in noise.
6. Output a prioritized list, Critical/High first.

Checklist

• AuthN/AuthZ — every new endpoint/action checks identity AND permission; no missing per-object authorization (IDOR); no trusting client-supplied user IDs, roles, or is_admin flags.
• Input validation — untrusted input validated/allow-listed at the boundary; length/type/format enforced.
• Injection — SQL/NoSQL parameterized; no string-built queries; no shell=True/os.system on user input; no command, LDAP, or template injection.
• SSRF — outbound URLs from user input are allow-listed; no fetching arbitrary user-supplied URLs/IPs (block internal ranges & metadata endpoints).
• Insecure deserialization — no pickle/yaml.load/Java native deser on untrusted data; use safe loaders.
• Secrets — no hardcoded keys/tokens/passwords; config from env/secrets manager.
• Crypto — strong algorithms (AES-GCM, SHA-256+, bcrypt/argon2); no MD5/SHA1 for security; TLS verification on; secrets not random for tokens.
• Error handling / info leak — no stack traces, SQL, or secrets in responses/logs; fail closed; generic error messages to users.
• XSS / output encoding — output encoded for its context; no dangerouslySetInnerHTML/innerHTML with untrusted data; safe templating.
• Access control on files/paths — no path traversal (../), uploads validated for type/size and stored outside the web root.

How to fix

• For each finding give the specific remediation: parameterize the query, add the authz check at line X, swap random → secrets, allow-list the URL host, switch yaml.load → yaml.safe_load, encode the output.
• Recommend the framework-native control where one exists (the ORM's binding, the framework's CSRF token, the platform's secrets store).
• Flag anything that needs a follow-up ticket vs. must-block-merge.

Report back

Output findings grouped by severity (Critical → Low), each with file:line, one-line risk, and the fix. Lead with a verdict: "block merge — N critical/high issues" or "approve — only low-severity notes". Note what you reviewed and any areas you couldn't fully assess.