Secure Code Review

Secure code review

A reviewer who reads files in the order the diff lists them finds what a linter finds. The value is in reading in the order defects cluster, following untrusted input to where it does damage, and writing findings precise enough that a fix lands in the right place.

When this fires

A change is about to ship and it touches authentication, authorization, money, personal data, tenancy, uploads, deserialization, URLs or secrets; or a repository needs a security pass. Also on a report that someone reached something they should not have.

Procedure

1. Fix the scope and the ground rules in writing. What is in scope, what the code is supposed to do, and — explicitly — that this is a reading exercise. Running the application, sending payloads, scanning a host or touching production is a separate, authorized act: propose it and stop and ask, naming the target and the environment.
2. Get the ground truth. For a change: the actual diff, plus the callers of anything it changed — a guard removed in one file is a defect in every file that relied on it. For a repository: the route table, the entry points, and the dependency manifest.
3. Read in the order defects cluster, not in file order. New or changed entry points; anything touching identity or permissions; anything building a query, command, path, URL or template; anything handling money, credentials, personal data or tenant scope; configuration and middleware (ordering, CORS, cookie flags, content security policy, dependency bumps); and last, everything labelled temporary, commented out, or added late under pressure.
4. Trace each candidate both directions. Backwards to an untrusted source, forwards to the dangerous sink, and check what the path in between actually enforces. A finding is a source, a sink, and a path. Without the path it is a hardening suggestion — still worth saying, but say which it is. Use the owasp-web skill for the class-by-class patterns.
5. Check the cases the author did not write a test for. The other tenant, the anonymous caller, the expired or replayed token, the second identical request, the bulk endpoint, the id belonging to a different scope, the empty and the enormous input.
6. Ask what changed about the rules, not only about the lines. A relaxed database constraint, a middleware registered earlier or later, a default that flipped, a dependency major version, a feature flag now on by default. These are invisible in a diff read line by line and they move the security properties of code nobody touched.
7. Do not turn into a scanner. A tool hit you cannot connect to a reachable path is a lead to triage, not a finding to report. Noise trains the reader to skim the one finding that mattered.
8. Say exactly what you did, per finding. Read, executed, tested, reproduced — these are different claims. "Reachable by inspection" is honest; "exploitable" requires having run it, in an environment you were authorized to run it in, and you name that environment.
9. Group by root cause, not by occurrence. One unscoped shared query reachable from four handlers is one finding with four locations and one fix — at the choke point every caller goes through. Four findings invite four local patches and leave the fifth caller open.
10. Write each finding so it can be fixed without you: file and line, the untrusted input, the path to the sink, the concrete consequence to a named asset, the smallest correct fix, and your confidence with what evidence would settle it.
11. Rank by consequence and reachability. Unauthenticated, no preconditions, hits credentials or the whole dataset, at the top. A category name is not a severity: not every injection outranks every access-control gap, and usually the reverse.
12. Report coverage and blind spots in the same document. What was read, what was skipped, what could not be assessed and why. A review with no stated limits reads as a clean bill of health it did not earn.
13. Fixing and disclosing are separate decisions. Proposing a patch is part of the review; committing it, pushing it, or writing an unfixed vulnerability into a public issue or PR comment is outward-facing — stop and ask before any of those.

Checklist

• Scope agreed, and no running system touched without an explicit ask
• Diff read together with the callers of everything it changed
• Clustered reading order followed, config and middleware included
• Each candidate traced source → path → sink; hardening labelled separately
• Negative cases considered: other tenant, anonymous, replay, bulk, expired
• Rule changes (constraints, defaults, ordering, dependency majors) examined
• Tool output triaged for reachability, not pasted
• Read / executed / tested / reproduced distinguished in every claim
• Findings grouped by root cause, with the fix at the shared choke point
• Severity argued from consequence and reachability
• Coverage and blind spots stated
• Nothing committed, pushed or published without asking

Failure handling

• You cannot tell whether input is trusted — say so and name what would settle it (the caller, the middleware, the schema). An assumed-safe input is how the interesting bugs survive review.
• The path runs through code you cannot follow — dynamic dispatch, generated code, a vendored binary — report the trace as far as it goes and mark it unresolved. Unresolved is not safe.
• The fix is architectural and large — size it and say so rather than proposing a local patch that adds a second enforcement point. Two checks that can disagree are worse than one badly placed.
• You find something serious in unrelated code — report it; do not quietly widen the change. Scope is about what you touch, not about what you are allowed to notice.
• Nothing was found — say what was read and what was not. "No findings in the six handlers and the auth middleware; the queue consumers and the infrastructure config were not reviewed" is a result. "Looks fine" is not.
• The change is already merged or deployed — that changes urgency and sequencing, not the review. Report first, and let the people who own the system decide on rollback or a fix forward.

Evidence to report

The scope and what was read, by file or area; per finding, the location, the source-to-sink trace, the consequence, the fix and the confidence; the severity ranking with its reasoning; tool output with its triage; the explicit list of what was not reviewed and what could not be assessed; and, for anything you ran, exactly what you ran and against which environment.