Manual Code Review

A scanner finds patterns. A reviewer finds the thing the pattern was standing in for — the missing ownership check, the workflow that can be entered at step three, the sanitiser applied to the wrong context. This page is the pass you make with your own eyes, and how to make it repeatable.

Review plan

A review without a plan becomes a tour of the files that looked interesting. Write the plan before opening the repository, and order it by consequence rather than by directory: what is internet-facing, what touches money or identity, what handles untrusted files. The time-box matters as much as the order — an unbounded review of a large codebase stops at 60% of the authentication module and never reaches the payment path.

A plan that holds up has five lines: scope (which repositories and branches), entry points (what can be called from outside), the highest-consequence flows (login, payment, admin, file handling), the language and framework versions in use, and what is explicitly out of scope. That last line is what stops a reviewer from being asked why they did not audit the vendored library.

Architecture and trust boundaries

Every finding needs an attacker on one side of a boundary and a capability on the other. Draw the boundaries before reading the code: browser to API, API to database, API to internal service, worker to queue, admin console to everything. Then, for each crossing, ask what the receiver validates and who it trusts to have validated already. The defects cluster exactly where two teams each assumed the other had checked.

Authentication and authorisation

Broken access control has been the top entry in the OWASP Top 10 for two editions running, and in source it almost always looks like this: a handler that authenticates correctly and then fetches an object by identifier without asking who owns it. The route guard proves the caller is logged in. It says nothing about whether this invoice is theirs.

@RestController
@RequestMapping("/api/invoices")
public class InvoiceController {

    @GetMapping("/{id}")
    public Invoice get(@PathVariable Long id, Principal caller) {
        // authenticated, but whose invoice is it?
        return invoiceRepository.findById(id).orElseThrow();
    }
}

@RestController
@RequestMapping("/api/invoices")
public class InvoiceController {

    @GetMapping("/{id}")
    public Invoice get(@PathVariable Long id, Principal caller) {
        // authenticated, but whose invoice is it?
        return invoiceRepository.findById(id).orElseThrow();
    }
}

@RestController
@RequestMapping("/api/invoices")
public class InvoiceController {

    private final InvoiceRepository invoices;

    @GetMapping("/{id}")
    public Invoice get(@PathVariable Long id, Principal caller) {
        Invoice invoice = invoices.findById(id).orElseThrow();
        // ownership is part of the query, not a follow-up check
        if (!invoice.getOwnerId().equals(currentUserId(caller))) {
            throw new AccessDeniedException("not your invoice");
        }
        return invoice;
    }
}

@RestController
@RequestMapping("/api/invoices")
public class InvoiceController {

    private final InvoiceRepository invoices;

    @GetMapping("/{id}")
    public Invoice get(@PathVariable Long id, Principal caller) {
        Invoice invoice = invoices.findById(id).orElseThrow();
        // ownership is part of the query, not a follow-up check
        if (!invoice.getOwnerId().equals(currentUserId(caller))) {
            throw new AccessDeniedException("not your invoice");
        }
        return invoice;
    }
}

The same shape appears in every framework, and the fix is the same: make ownership part of the query where you can, and an explicit check where you cannot. What does not work is a check in the template, a filter on the list endpoint, or a client-side hide — all three are bypassed by calling the API directly.

Input handling

The most common review note in the world is "sanitise your input", and it is usually wrong. Input validation is an allowlist at the point where a value enters the system — type, length, range, format. It cannot make a value safe for SQL, HTML and a shell at the same time, because those three contexts have incompatible escaping rules. Encoding belongs at the sink, chosen for that sink's context.

// one sanitiser, three contexts, no idea which one is right
String clean = input.replace("<", "").replace(">", "");

String html  = "<div>" + clean + "</div>";
String sql   = "SELECT * FROM users WHERE name = '" + clean + "'";
String shell = "ping -c 1 " + clean;

// one sanitiser, three contexts, no idea which one is right
String clean = input.replace("<", "").replace(">", "");

String html  = "<div>" + clean + "</div>";
String sql   = "SELECT * FROM users WHERE name = '" + clean + "'";
String shell = "ping -c 1 " + clean;

// encode per sink: HTML escape, bind parameters, pass argv
String html = "<div>" + HtmlUtils.htmlEscape(input) + "</div>";

String sql = "SELECT * FROM users WHERE name = ?";
PreparedStatement ps = conn.prepareStatement(sql);
ps.setString(1, input);

// no shell string at all: argv, no interpretation
Process p = new ProcessBuilder("ping", "-c", "1", input).start();

// encode per sink: HTML escape, bind parameters, pass argv
String html = "<div>" + HtmlUtils.htmlEscape(input) + "</div>";

String sql = "SELECT * FROM users WHERE name = ?";
PreparedStatement ps = conn.prepareStatement(sql);
ps.setString(1, input);

// no shell string at all: argv, no interpretation
Process p = new ProcessBuilder("ping", "-c", "1", input).start();

Read for the inverse too: code that escapes on the way in and then stores the escaped value, which double-encodes on the way out and corrupts data. Validation is a property of the boundary; encoding is a property of the sink.

Crypto and secrets

In review, crypto findings are almost never about the mathematics. They are about four repeats: a key in the source, a hash chosen for passwords (MD5, SHA-1, or unsalted SHA-256), a cipher mode without authentication, and a random source that is not a CSPRNG. Searching for these four is faster by grep than by reading, and the hits are high-signal because production code rarely contains a hash function by accident.

Business logic

Logic flaws have no sink to grep for. They appear where the code trusts something it should recompute: a price sent by the client, a discount applied twice, a workflow step reachable without its predecessors, a balance updated without a lock. Read these flows as a state machine and ask what state the attacker can force.

Evidence Collection

Bedrock Questions Before Filing

Manual Review Checklist

Fixing What You Found

Reporting findings

The review is only as good as what survives it. Two reviewers looking at the same defect produce different tickets, and the difference is whether the ticket carries a path. Source line, sink line, request, preconditions, and the control that was assumed to be there — that is the whole of it. The rest of the lifecycle, from severity to the regression test that closes the finding, is on Triage and Remediation.