Secure code review, and what a reviewer looks at first

Why is line-by-line review the wrong start?

Because it spends the reviewer's scarce attention on the categories a machine already covers, and exhausts it before reaching the categories a machine cannot. Static analysis is better than any human at finding a string concatenated into a query across ten thousand files. A human is the only thing that can notice that an endpoint returning invoices never restricts them to the caller's organisation. Sequential reading arrives at the interesting question with nothing left.

It is also poorly matched to how codebases fail. Vulnerabilities cluster at boundaries, where data crosses from untrusted to trusted, from one privilege level to another, or from one service to another with an assumption of trust attached. The interior of a well-factored module is rarely where the problem is, and reading it in order means reading mostly interior.

So the first hour goes on enumeration rather than reading. What are the entry points, what identity does each run as, and where is the decision made about what that identity is allowed to touch. If a reviewer cannot answer those three after an hour, that is itself the finding, because an application whose trust boundaries are not discoverable by a competent reader is not one whose authorisation can be relied upon.

What does a reviewer open first?

The route table, or whatever the framework's equivalent is, because it is the inventory of ways in. Then the middleware or filter configuration that decides which of those routes require authentication, and in what order it runs. The specific thing to look for is whether the default is deny: can a developer add a route and have it be public by accident, or does registration without an explicit policy fail? Frameworks differ, and the answer determines how much of the rest matters.

Entry points are wider than HTTP and the forgotten ones are where problems live. Message queue consumers, scheduled jobs, webhook receivers, GraphQL resolvers that bypass the REST middleware, internal service endpoints that assume the network is trusted, administrative command line tools, and file import paths. Each of these is a place where input arrives, and several of them commonly have no authorisation layer at all because the network was assumed to be the boundary.

Then the data access layer, looking for how a query is scoped to a tenant or an owner. The best answer is that scoping is structurally impossible to omit, because every query goes through a repository that requires an actor. The common answer is that each handler adds a filter by hand, in which case the review becomes an exercise in finding the handler where somebody forgot.

Which checks give the most per minute?

A short list of searches, each of which is a one-line query against the repository and each of which routinely finds something. None requires understanding the whole application, which is why they are the right way to begin an unfamiliar codebase.

What does a good reviewer look for that tools cannot see?

Check-then-act sequences on anything that represents value. A handler that reads a balance, decides it is sufficient, then writes a decrement, is correct in a single-threaded world and wrong under concurrency. The fix is a database-level guarantee, whether a conditional update, a row lock or a constraint, and the absence of one is invisible to a scanner because every individual statement is fine.

Cryptographic misuse in the small. Random values used as security tokens but drawn from a general-purpose random generator rather than a cryptographic one. Comparisons of secrets with an ordinary equality operator instead of a constant-time function. A password stored with a fast general hash rather than a purpose-built password hash with a work factor. Each is a few characters of code and each defeats the control it appears to implement.

And the state machine. Ask which order the steps of a paid or privileged workflow must occur in, then look for whether anything enforces it. If the answer is that the front end only offers the buttons in the right order, an attacker who calls the endpoints directly can skip payment, reopen a closed dispute, or resubmit an approval, and no tool will report it because each endpoint does exactly what it says.

What should be reviewed on every pull request?

A change-shaped list rather than a full review, and it should be short enough that people actually apply it. Does this change add a route, a consumer or a job? Does it change who can call something? Does it introduce a new query, a new outbound request, a new file path, a new deserialiser, or a new dependency? If all six answers are no, security review adds nothing and should not be requested.

The value of framing it this way is that it makes the trigger objective. Teams that ask for security review on everything get performative approvals; teams that ask on nothing get surprised. A rule tied to the presence of specific constructs in the diff, ideally enforced by a code-owners file that pulls in a reviewer when particular paths change, generates review requests that are worth someone's time.

Pair it with one standing question in the pull request template: what could a malicious authenticated user do with this endpoint that the feature does not intend? Written answers to that question, from the author, are consistently more useful than a reviewer's guess, because the author knows the intent and the reviewer is reconstructing it.

When is external review worth paying for?

When the code implements a trust decision that is expensive to get wrong and rare enough that your team has done it once. Authentication flows, multi-tenant isolation, payment and settlement logic, permission models, licence or entitlement enforcement, and anything cryptographic. These are the areas where experience compounds and where a reviewer who has seen twenty implementations sees the failure immediately.

It is poor value for general codebase sweeps, because an external reviewer without domain knowledge cannot tell an intentional design from a mistake, and the report comes back full of questions rather than findings. If you want breadth, buy targeted testing against a running system instead. If you want depth on a boundary, buy review with the design document attached.

The practical arrangement that works is a scoped review of named files plus a walkthrough with the engineer who wrote them. An hour of conversation before the reading starts changes what the reviewer can find, because most of the interesting questions are about intent, and intent is not in the repository.