Secure Code Review skill for Codex

A security review of a change answers one question per hunk: can untrusted input reach a dangerous operation here without a control in between? Review the diff in the context of the code it touches, not in isolation. Flag only what you can justify; every comment names the risk, the evidence, and the fix.

Scope the review

git fetch origin && git diff --stat origin/main...HEAD      # what changed
git diff origin/main...HEAD                                 # the change itself

Read AGENTS.md for stack and conventions. Then classify the diff — a 3-line config change and a new auth flow get very different scrutiny:

Diff touches	Review depth
Auth, sessions, tokens, access control	Highest — read the whole surrounding flow
Raw SQL/query building, templating, shell, deserialization	High — trace every input
File upload/download, path handling, URL fetch	High — SSRF/traversal
Crypto, secrets, env handling	High — primitives and storage
Dependency bumps / lockfile	Check changelog + advisories (hand off to dependency-scanning)
Pure UI, copy, formatting	Light — confirm no injected markup sink

The one question, applied

For each changed hunk that handles data, trace: source → transformations → sink.

• Sources (untrusted): request params/body/headers/cookies, uploaded files, webhook payloads, queue messages, DB rows that originated from users, third-party API responses.
• Sinks (dangerous): SQL/NoSQL queries, OS commands, eval/dynamic code, HTML/template output, file paths, outbound URLs, deserializers, redirects, log lines with secrets.
• Controls (what must sit between): parameterization, output encoding, allowlist validation, authorization check, path canonicalization, size/type limits.

A finding is: a source reaching a sink with no adequate control on the path. If a control exists but is weak (blocklist instead of allowlist, validation that can be bypassed), say so specifically.

High-signal checks for a diff

• New route/handler: is it behind the same auth/authz as its siblings? New endpoints are a classic place where an access-control gate is forgotten.
• Changed query: did a parameterized query become string-built, or did a new column come from user input? git diff makes this visible — look for added + / ${} / f-strings near query calls.
• Object lookup by ID: is there an ownership/tenant filter? (IDOR)
• Redirect/Location from input: open-redirect; require an allowlist or relative path.
• New dependency or CI action: pinned to a version/SHA, from a trusted source?
• Error handling: does a new catch leak stack traces or internal detail to the client?
• Secrets: any key, token or password added inline or in a committed .env? Flag and say rotate — a committed secret is compromised even if removed later.
• Logging: does a new log line write a token, password, full card number or PII?
• Regex on input: could it catastrophically backtrack (ReDoS)? Flag nested quantifiers.

Comment style

Each comment: severity tag, one-line risk, the evidence from the diff, the fix. Match the severity bands your team uses; default to Critical / High / Medium / Low / Nit. Keep the tone about the code. Example:

[High] IDOR — `invoices/:id` is read without an owner check.
At routes/invoices.ts:+58, `Invoice.findByPk(req.params.id)` returns any invoice to any
authenticated user. Scope it: `findOne({ where: { id, orgId: req.user.orgId } })`.

Separate “must fix before merge” (Critical/High) from “should fix” (Medium) and “nit”. Do not block a PR on style under a security label.

Verification

• For each High+ finding, state the concrete abuse in one sentence; if you cannot, downgrade to “needs verification” rather than asserting it.
• Run the project’s linters/SAST if present (semgrep, npm audit, etc.) and reconcile their output with your manual findings — do not duplicate, do not blindly trust.
• After fixes, re-diff and confirm the control is actually on the path, not just added nearby.

Output contract

## Security review — PR #<n>: <title>
Verdict: <block | approve-with-fixes | approve>
Scope: <files reviewed> (<n> of <m> changed files; rest are <reason>)

### Must fix before merge
- [High] <risk> — `file:line` — <evidence>. Fix: <fix>.

### Should fix
- [Medium] ...

### Nits / notes
- ...

Positive: <controls done right, so they are not removed later>

Hard rules

• Review what changed and its immediate context; do not rubber-stamp a large diff you did not read. Say which files you did not cover.
• Never approve a change that adds a committed secret — require rotation.
• Do not run exploit payloads against live systems from a review; this is static work.
• If the diff is too large to review safely, say so and ask to split it.