security-diff-review-method

Security Diff Review Method

Overview

You are reviewing a change, not auditing a repository. The question is: what can an attacker do after this diff that they could not do before? Every finding answers it with a source you can point at, a sink you can point at, and the diff line that connects them.

Core principle: risk first, then evidence. Spend your reading on the files that can hurt someone; trace one real data flow end to end rather than skimming ten.

1. Size Up the Change

The injected diff starts with git diff --stat — every file it lists is under review. Get the base once and reuse it:

base=$(git merge-base HEAD origin/HEAD 2>/dev/null || git merge-base HEAD origin/main)
git diff --stat "$base"
git diff "$base" -- path/to/file      # each file the 24,000-byte cut left out

Tier every file before reading any of them closely:

Tier	What lands here	Depth
HIGH	auth/session, access checks, crypto, secrets, deserialization, file paths, outbound URLs, shell/exec, SQL/templates, payments, dependency manifests and lockfiles, CI workflows, Dockerfiles, IaC, agent/MCP configs, anything an LLM with tools reads	full source → sink trace
MEDIUM	business logic, state changes, new public endpoints or exported functions, new config	trace the inputs it accepts
LOW	comments, copy, styling, tests, logging that carries no secret	scan for secrets only

A refactor is HIGH until you have shown it is LOW: moved code is where a guard silently stays behind.

2. Phase 1 — Repository Context

Before judging new code, learn how this repository already defends itself. A finding is strongest when it says "every neighbour does X, this one does not".

• Auth and access: the middleware group, the @PreAuthorize/@RolesAllowed convention, the "load for owner" repository method, the tenant filter.
• Input handling: the validator, the query builder, the HTML escaper, the path helper, the URL fetcher with an allowlist.
• Secrets: how config is loaded (env, secret manager), what the logger redacts.
• Deployment shape: get_project_brief — a public multi-tenant service, an internal tool and a local desktop app have different attackers. It changes severity, never whether you trace.