Reviewing Code Changes

Reviewing Code Changes

Reviewing a diff is not auditing a codebase in miniature. The diff shows you what changed but hides what the change means — the caller that now receives untrusted data, the check that used to run, the assumption that no longer holds. Reviewing only the added lines is the standard way to miss the bug.

When to Use

• Security review of a pull request, branch, or commit range
• Pre-commit or pre-merge check on your own changes
• Assessing risk of a dependency bump or config change
• Reviewing an AI-generated change before it ships

When NOT to Use

• Whole-codebase audit — use auditing-code-for-vulnerabilities
• Runtime testing of the deployed change — use the relevant testing skill
• Dependency/CI changes as the main subject — use auditing-supply-chain

Get the Real Diff First

# Against the actual merge base, not against whatever HEAD happens to be
BASE=$(git merge-base HEAD origin/main)
git diff --stat $BASE...HEAD
git diff $BASE...HEAD

# Rename and whitespace noise hides real changes
git diff -M -C -w $BASE...HEAD

# Files changed most, and by whom — where to spend attention
git diff --numstat $BASE...HEAD | sort -rn | head -20

Three things to establish before reading code:

1. What is this change trying to do? Read the PR description and the tests. A security review without the intent is pattern matching.
2. Does the diff match the description? Unexplained files — a config change, a new dependency, a workflow edit riding along with a feature — are the highest-signal thing in a review.
3. What is the change's exposure? A change to an unauthenticated request path deserves an order of magnitude more attention than an internal refactor.

Triage: Where to Spend the Review

Not all diffs deserve equal time. Rank hunks by what they touch:

# Fast triage of a large diff
git diff $BASE...HEAD --name-only | rg 'auth|login|session|crypto|password|token|admin|permission|\.github/workflows|Dockerfile|terraform|policy'
git diff $BASE...HEAD -U10 | rg -n '^\+.*(eval\(|exec\(|system\(|innerHTML|pickle|yaml\.load|Sprintf.*SELECT|shell=True|verify=False|InsecureSkipVerify)'

What to Look For

Removals and weakenings — read the minus lines first

The most dangerous diffs delete things. Search the removed lines specifically:

git diff $BASE...HEAD | rg '^-' | rg -i 'auth|verify|valid|check|sanitiz|escape|permission|assert|csrf|limit'

• A validation call deleted "because it was redundant"
• An authorization decorator removed during a refactor
• A test deleted rather than fixed
• A TODO: add authz that was never done and is now shipping
• A timeout, size cap, or rate limit removed
• strict mode, a security header, or a CSP directive relaxed

Additions — the ordinary classes, in context

Apply the bug-class hunting from auditing-code-for-vulnerabilities, but scoped to the changed paths: injection sinks, authorization on new object lookups, SSRF in new outbound calls, path handling on new file operations, deserialization, secrets, weak randomness, unsafe defaults.

The two questions that catch most real issues in a diff:

Does new untrusted data reach an old sink? Does old untrusted data reach a new sink?

Both require reading outside the diff.

Context beyond the diff

Read the whole function for every non-trivial hunk. Then check callers:

# Who calls the changed function, and do they still satisfy its assumptions?
rg -n 'changedFunctionName\s*\(' --type <lang>
# What else in the codebase uses the pattern this change introduced?
git log -1 --format=%H $BASE  # anchor, then compare behaviour before/after

Specifically check whether the change alters an invariant that other code relies on: a function that used to sanitize and now does not, a return value whose meaning flipped, a nullable that became non-null, an ordering guarantee that was dropped.

Config, infra, and pipeline changes riding along

git diff $BASE...HEAD -- '*.yml' '*.yaml' '*.tf' 'Dockerfile*' '.github/**' '*.json'

• New or bumped dependencies: who maintains them, and does the lockfile change match the manifest change? (See auditing-supply-chain.)
• Workflow permissions widened, pull_request_target added, unpinned actions
• Security group / firewall rules opened, buckets made public, IAM "*"
• Debug flags, verbose logging of request bodies, disabled TLS verification
• Feature flags that default to on