how to review a PR for security issues quickly

how to review a PR for security issues quickly

TL;DR

A useful security review takes fifteen minutes, not fifteen hours: scope the diff to what changed, then run a fixed checklist over auth, input handling, secrets, data exposure, and dependencies. You are looking for bug classes, not proving exploits. Anything you cannot clear in the pass becomes a question for the author, not a shrug.

how to review a PR for security issues quickly

Use this when

• You are the reviewer on a PR and want a repeatable security pass
• An agent is doing automated first-pass review before humans look
• Your team keeps shipping the same bug classes and wants a checklist
• A large PR needs triage into "safe" and "needs a closer look"
• You are onboarding reviewers who are not security specialists

Not for this skill when

• You need a full penetration test (different engagement entirely)
• You are threat modeling a new system from scratch
• The review is about performance, style, or architecture
• The code is already in production and suspected compromised (incident response)

Steps

1. Scope the diff before reading code

Look at the file list first. Mark anything touching auth, sessions, crypto, payments, PII, outbound requests, or dependency manifests as high-attention. A ten-file PR with one auth file means nine files get a skim and one gets scrutiny.

git diff --stat main...HEAD | tail -20

Expected: you can name the two or three files that deserve deep review before reading further.

2. Check authentication and authorization changes

For every changed access check, ask: who can reach this now that could not before, and who can no longer reach something they should. Missing checks fail open far more often than extra checks fail closed.

Expected: each new endpoint or permission change has an explicit test proving the unauthorized case is rejected. "It worked when I clicked it" is not that test.

3. Trace untrusted input to its sink

Pick the inputs the PR introduces (form fields, query params, headers, file uploads, webhook bodies) and follow each to where it is used: SQL, HTML, shell, URLs, logs. One input, one sink, one verdict each.

Expected: every input has a named sink and a control (parameterization, encoding, allowlist). An input whose sink you cannot identify is a question for the author.

4. Hunt for secrets and credentials

Scan the diff for tokens, keys, passwords, and connection strings, including in tests, fixtures, and comments. Check that new config reads from the environment or secret manager rather than hardcoding.

git diff main...HEAD | grep -in "token\|secret\|password\|api.key" | head -20

Expected: the grep hits are all references by name (like "read from the vault path") rather than literal values. A literal credential in the diff blocks the PR until it is rotated and purged from history.

5. Check data exposure in responses and logs

New endpoints and new log lines are where PII leaks. Confirm responses return only the fields the client needs and that logs do not include tokens, passwords, or full user records.

Expected: you can point at the response shape and the log statements and say what is excluded. Debug logging left at verbose in a hot path is a finding.

6. Review dependency and config changes

New packages and version bumps get a quick reputation check: is it the real package, is the version sane, does the lockfile match. Config changes get checked for debug flags or relaxed TLS.

Expected: the lockfile matches the manifest, and no debug or permissive setting is riding along unexplained.

7. Leave findings as questions with a fix sketch

"Is this query parameterized? If not, passing the values as bindings fixes it" gets faster, friendlier fixes than a bare "SQLi". End the review with the count: findings, questions, and what you checked and cleared.

Expected: the author can act on every comment without a follow-up meeting.

Variant: reviewing agent-generated PRs

Agent-written code is syntactically confident and semantically optimistic, which means the checklist matters more, not less. Pay extra attention to input handling and auth, the two areas where generated code most often looks right while being wrong.

Variant: reviewing dependency-update PRs

Dependabot-style PRs look safe but can carry confusion attacks or compromised packages. Check the version jump, skim the changelog, and confirm the lockfile matches before approving on sight.

Variant: the five-minute emergency review

When there is no time for the full pass, do steps 2 and 4 only: auth changes and secrets in the diff. Those two catch the findings that become incidents.

Why this happens

Reviewers default to checking correctness and style because those are visible, while security flaws hide in the paths nobody traced. A fifteen-minute budget works because most PRs only introduce a few new trust decisions, and those are what you are hunting.

Edge cases and pitfalls

• Large PRs defeat checklists; ask the author to split them rather than reviewing 2000 lines superficially.
• Test files get less scrutiny but often contain real credentials and realistic PII; review them too.
• Approving with "fix in a follow-up" for security findings means the finding ships; block or accept consciously.

Provenance

Resolved from the public thread: https://vectle.com/posts/psts3-4ymORVVQI54DDZoJFw