Secure code review that changes the code

Why findings reports do not change code

The standard secure code review deliverable is a document: an executive summary, a methodology section, and a table of findings ranked by severity with file and line references. It is professional, thorough, and in most organisations it produces a low single-digit percentage of remediation within the quarter.

The reasons are structural rather than cultural. A finding described in a document has to be re-understood by an engineer who was not in the review, re-located in a codebase that has since moved, re-prioritised against feature work by someone who cannot evaluate the security argument, and re-tested by whoever picks it up. Each step loses volume. By the time the finding reaches a sprint, it is a line item with a severity label and no context.

A review that intends to change code has to compress that chain. The unit of delivery is not a finding; it is a change the team can merge.

Scope by attack path, not by directory

Even-coverage reviews spend most of their effort on code where a defect has no consequence. Scope instead from the threat model: identify the paths from an untrusted input to a consequential action, and read the code on those paths thoroughly.

Concretely, that means enumerating:

• every entry point that accepts data from outside the trust boundary, including webhooks, message consumers, file imports and administrative interfaces;
• every point where that data influences an authorisation decision, a query, a filesystem or network operation, a template, a deserialisation, or a command;
• every consequential action—money moved, data exported, permission granted, device instructed—and the shortest path to it from any entry point.

This produces a smaller and better-chosen reading list than a directory walk, and it makes the exclusions explicit. A review that states “the reporting module was not examined because no consequential action is reachable from it” is more useful than one that implies uniform coverage it did not have.

Automated analysis still runs across everything. The two are complementary: tooling covers breadth and known weakness classes, human reading covers the logic that tools cannot model. NCSC’s development guidance makes the same division—use tooling where it is reliable, and spend scarce human attention where judgement is required.

Read the code that decides, not the code that computes

Most of a codebase transforms data. A small fraction decides something: whether a request is permitted, which tenant a query targets, whether a signature validated, which branch handles an error. Security defects concentrate almost entirely in the deciding code.

Practically, this narrows reading to a few shapes:

Shape	What to look for
Authorisation checks	Missing on some paths; performed before the object is loaded; using a client-supplied identity
Boundary parsing	Validation after use; canonicalisation order; type confusion on optional fields
Query construction	String concatenation; a tenant predicate that is optional; a filter applied after retrieval
Cryptographic use	Hard-coded keys; a mode that provides confidentiality but not integrity; a comparison that is not constant-time
Error handling	A failure path that continues; an exception that exposes internal state; a default that is permissive
State transitions	A check performed on stale state; an operation that is not idempotent where replay is possible

The last two rows produce the findings that automated tools miss most often, because “the permissive default” and “the check that ran against state fetched three calls earlier” are correct code in isolation. Naming the class precisely—using CWE identifiers where they fit—makes the finding actionable and keeps recurrence countable.

Report patterns rather than instances

Thirty-one instances of missing output encoding are one finding, not thirty-one. Reporting them individually inflates the document, fragments the remediation into thirty-one tickets, and hides the fact that the real defect is the absence of a shared encoding mechanism.

Group findings by their cause and report the pattern: what the underlying gap is, where it manifests, what the single change that eliminates the class would be, and which instances need individual attention because they differ. This reframes remediation from a queue of small tasks into one design change with a follow-up sweep, which is both cheaper and more durable.

It also changes the conversation with the team. “You have thirty-one cross-site scripting findings” invites defensiveness. “Your templates escape by default except in three helper functions, and here is the change to those helpers” invites a merge.

Deliver a fix and a failing test

For at least one instance of every pattern, the review should hand over a branch: a test that fails against the current code, the change that makes it pass, and a note on what the change does not cover.

The failing test is the important half. It converts a security argument into a build result, which is the form in which engineering organisations act. It also survives the engagement—the test stays in the suite after the reviewer has gone, and it is what prevents the pattern returning.

Where the fix belongs in a shared component, propose it there rather than at the call site. The OWASP proactive controls argument applies: a control implemented once in a platform library and used everywhere is reviewable, while the same control restated per call site is a permanent source of omissions.

Some findings resist this. A design-level problem—an authorisation model that cannot express the required rule, a protocol that lacks replay protection—cannot be fixed in a patch. Say so plainly, describe the options with their costs, and route it to the architecture decision rather than the backlog.

Make the fix hard to undo

A remediated defect that can return has been postponed rather than fixed. Three mechanisms close the loop, in increasing strength:

• a regression test asserting the negative case, which catches an accidental reintroduction;
• a lint or static rule that rejects the unsafe construct, which catches a deliberate one written in ignorance;
• an interface that makes the unsafe construct unavailable—removing the raw query method, making the authorisation context a required parameter—which prevents it entirely.

The third is worth the refactoring cost for classes that have recurred. If output encoding has been fixed twice and returned twice, the problem is not the developers; it is that unsafe rendering remains callable.

Track this the way the SSDF frames vulnerability response: identify the root cause, and assess whether comparable defects exist elsewhere and whether the process that admitted the defect needs changing. The second half is the one that compounds.

What to count afterwards

Findings per engagement is the wrong metric; it rewards verbosity and falls as a codebase improves, which looks like declining review quality. Three numbers are more informative.

The first is remediation rate by pattern within an agreed window—what proportion of reported classes were closed, not what proportion of instances. The second is recurrence: how often a class that was fixed appears again in later releases, which tests whether the fix was structural. The third is the proportion of findings that automated analysis could have found, which tells you whether human review time is being spent where only humans can help.

A review that reports fewer findings each cycle while recurrence stays near zero is working. A review that reports a stable number of the same classes every year is producing documents, and the verification standard it was measured against is not the reason.