L58 · A secure code review checklist

Module 8: Secure SDLC and Supply Chain · ~20 min · Lesson 58 of 71

🎯 What you'll learn

• A repeatable way to read a pull request for security: follow untrusted data from where it enters to where it lands
• A nine-area checklist that fits on one screen, each area linked to the lesson that teaches it
• How to spot what a diff doesn't show: removed checks, new routes, changed defaults
• Practice on an annotated Spring diff with nine planted issues, each with its line and fix

🔥 Why this matters

A 40-line PR adds "export my statements" to statements-svc. Tests pass, SAST is green, and a teammate approves it in three minutes with "LGTM". But the endpoint never checks that the account ID in the URL belongs to the caller: BOLA (L40), the top API risk. Scanners (L54) rarely catch it, because the code looks fine; only a reviewer who asks "whose account is this?" does.

1. Read for data flow, not line by line

Reading a diff top to bottom makes you check style. For security, read it as a map of sources (places untrusted data enters) and sinks (places where data can do damage if it's wrong).

 SOURCES (untrusted)            SINKS (where it can hurt)
 path & query params ──┐   ┌──▶ Mongo / SQL query      (L30, L31)
 JSON body ────────────┤   ├──▶ HTML / template        (L32)
 headers, cookies ─────┼─▶ ┼──▶ outbound URL fetch     (L35)
 JWT claims ───────────┤ ? ├──▶ deserializer           (L36)
 Kafka / S3 / API data ┘   ├──▶ log line               (L60)
                           └──▶ HTTP response, file, shell
                  ? = validated, authorized, encoded?

For every arrow the PR adds or changes, ask three questions. Is it validated (type, length, format)? Is the caller authorized for this object? Is it encoded or parameterized for that particular sink? Data from another internal service counts as a source too (API10, L39).

A good order for a review:

1. Read the description and ticket. What is this supposed to do, and for whom?
2. List the entry points. New routes, consumers, scheduled jobs, CLI flags.
3. Trace each source to its sinks with the three questions.
4. Look for what's missing (section 3).
5. Check the non-code files: dependencies, config, IaC and k8s manifests, workflows.

2. The checklist

Nine areas. You won't need all nine for every PR, but you should consciously skip each one.

Two habits make the checklist work. First, read beyond the diff: open the whole file, or the caller of the changed function, because the missing check is usually in code the PR didn't touch. Second, know what your tools already cover. Secret scanning, SCA and SAST (L54) catch patterns. Humans catch meaning: whose data, which role, what should happen on failure.

3. Read for what's missing

The most dangerous changes are often deletions or additions to config, which reviewers skim.

 BEFORE (main)                       AFTER (this PR)
 ───────────────────────────────     ───────────────────────────────
 .requestMatchers("/actuator/        .requestMatchers("/actuator/
   health").permitAll()                **").permitAll()          ← !
 @PreAuthorize("hasRole('OPS')")     (line deleted)              ← !
 test_other_users_account_is_403     (test deleted, "flaky")     ← !
 timeout: 5s                         timeout: 60s

Each line on the right "just makes it work", and each one widens access. The first now exposes /actuator/env and /actuator/heapdump if they're enabled (L38). Watch for:

• Deleted authorization annotations, checks, tests or validation.
• Widened patterns: route matchers, CORS origins (L37), IAM resources, NetworkPolicy selectors.
• New entry points that the security config may not cover, such as a new controller outside the package that's protected, or a new gateway route (Kong in the examples) without the auth plugin (L43).
• Changed defaults: required = false, a feature flag defaulting to on, verify=False.

4. Write comments that get fixed

A good security comment names the risk, shows how it's exploited, and offers a fix. "This might be insecure" starts an argument; this gets a fix:

Blocking (BOLA, L40): line 9 loads statements for any accountId in the URL. User A can read user B's statements by changing the ID. Suggest accessPolicy.requireOwner(jwt.getSubject(), accountId) before the query, plus a test where user A requests B's account and gets 403.

Label each comment blocking or non-blocking. If you aren't sure something is exploitable, ask a question instead of asserting.

🧪 Worked example: a diff with nine planted issues

Here's a PR for statements-svc. Read it once with the checklist before you scroll to the answers.

 1 +@RestController @RequestMapping("/v1/accounts")
 2 +class StatementController {
 3 +  static final String SFTP_PASS = "Sftp-Pa55-EXAMPLE";
 4 +  @Autowired MongoTemplate mongo;
 5 +
 6 +  @GetMapping("/{accountId}/statements")
 7 +  List<Statement> list(@PathVariable String accountId,
 8 +    @RequestParam String from, @AuthenticationPrincipal Jwt jwt) {
 9 +    log.info("export token={} acct={}", jwt.getTokenValue(),
10 +        accountId);
11 +    String q = "{accountId:'" + accountId
12 +        + "', date:{$gte:'" + from + "'}}";
13 +    return mongo.find(new BasicQuery(q), Statement.class);
14 +  }
15 +
16 +  @PostMapping("/{accountId}/statements/link")
17 +  String link(@PathVariable String accountId) {
18 +    String t = Long.toHexString(new Random().nextLong());
19 +    links.save(accountId, t);
20 +    return "https://files.example.com/dl/" + t;
21 +  }
22 +
23 +  @ExceptionHandler(Exception.class)
24 +  ResponseEntity<String> oops(Exception e) {
25 +    StringWriter w = new StringWriter();
26 +    e.printStackTrace(new PrintWriter(w));
27 +    return ResponseEntity.status(500).body(w.toString());
28 +  }
29 +}

   # deploy/statements-svc.yaml (same PR)
40       securityContext:
41 -       runAsNonRoot: true
42 -       allowPrivilegeEscalation: false
43 +       privileged: true      # "needed to debug"

The findings:

Finding 5 deserves a closer look. With from set to '}, date:{$ne:null}, accountId:{$ne:null}, x:{$ne:', line 12 builds:

{accountId:'ACC-1', date:{$gte:''}, date:{$ne:null},
 accountId:{$ne:null}, x:{$ne:''}}

The injected date and accountId keys come second. When the Java driver parses the JSON into a Document, the later duplicate key typically replaces the earlier one, so the filter now matches every account. Fix 3 alone would block this, because LocalDate can't contain quotes. Fix 5 makes it impossible even if someone later relaxes the type. That's defense in depth (L04).

The fixed handler:

@GetMapping("/{accountId}/statements")
List<StatementDto> list(@PathVariable String accountId,
    @RequestParam @DateTimeFormat(iso = DATE) LocalDate from,
    @AuthenticationPrincipal Jwt jwt) {
  accessPolicy.requireOwner(jwt.getSubject(), accountId); // 403
  log.info("statements.list sub={} acct={}",
      jwt.getSubject(), accountId);
  Query q = Query.query(where("accountId").is(accountId)
      .and("date").gte(from)).limit(500);
  return mongo.find(q, Statement.class).stream()
      .map(StatementDto::from).toList();
}

🛠 Try it (optional, ~5 min)

Pick a recent merged PR in a repo you work on and grep its diff (read-only; use your own PR number and repo):

P='permitAll|getTokenValue|printStackTrace|new Random\(|BasicQuery'
P="$P"'|password *=|privileged: true|"\*"'
gh pr diff 123 --repo your-org/your-repo | grep -nE "^\+.*($P)"

Each hit is a candidate, not a finding. Then walk the PR's entry points through the nine areas, noting which needed code outside the diff. That list is where reviews usually miss things.

🧠 Key takeaways

• Review for data flow: every source to every sink, asking "validated? authorized? encoded?"
• Use the nine-area checklist and consciously skip what doesn't apply.
• Deletions and config changes are where access quietly widens; read them twice.
• Scanners find patterns; reviewers find "whose data is this?" Authorization is your job.
• Comment with the risk, the exploit and the fix, and label it blocking or not.

📖 Words to know

⏭ Next time

L59 turns review findings into tests that keep them fixed: abuse-case unit tests, fuzzing and property-based testing.

You found the string-built BasicQuery in StatementController. The person who wrote it, or the example they copied, probably did the same thing somewhere else. Variant analysis means taking one confirmed bug and searching for every other place with the same root cause.

From one bug to a pattern

 1. confirmed bug        new BasicQuery("{accountId:'" + id + "'}")
          │
 2. root cause           Mongo filter built from a string with input
          │
 3. pattern              new BasicQuery(<anything but a literal>)
          │
 4. search               this repo ─▶ org's repos ─▶ shared libs
          │
 5. fix the class        safe helper + CI rule so it can't come back

Step 2 is the important one. "Line 11 is bad" finds nothing new. "We build filters from strings" gives you something you can search for.

Worked example: three levels of search

Grep is fast and finds the obvious cases in your own clone:

grep -rn --include='*.java' 'new BasicQuery(' src/ \
  | grep -v 'new BasicQuery("[^"]*")'

GitHub code search finds siblings across the org's repos (read-only; you only see repos you can already access):

gh search code 'new BasicQuery(' --owner example-org \
  --language java --limit 50

Semgrep understands syntax, so formatting and line breaks don't fool it. The rule below flags any BasicQuery whose argument isn't a plain string literal ("..." matches any literal):

rules:
  - id: mongo-basicquery-nonliteral
    languages: [java]
    severity: ERROR
    message: >-
      BasicQuery built from a non-literal string. Use
      Query/Criteria instead (NoSQL injection, L31).
    patterns:
      - pattern: new BasicQuery($Q)
      - pattern-not: new BasicQuery("...")

Run it locally with semgrep --config basicquery.yml --metrics=off src/. It will also flag a BasicQuery built from a constant variable. That's a false positive you can accept, because the safe helper is better anyway. For deeper data-flow questions ("does any request parameter reach this sink?"), CodeQL can run one query across many repos at once (GitHub calls this multi-repository variant analysis). L54 covers how these tools fit into CI.

Close the class, not the instance

When you find siblings, fix them together, then make the bug hard to write again:

1. A safe helper (StatementQueries.byAccount(id, from)) that people want to use.
2. The Semgrep rule in CI as a required check (L57), so a new instance fails the build.
3. One line in your team's review checklist: "Mongo filters built only with Criteria."

Security teams run this loop after every pen-test finding or bug-bounty report. Fixing one reported URL while three siblings stay open is a common way to get the same report twice.

🧠 Extra takeaways

• Every confirmed bug is a search query: name the root cause, then hunt for it.
• Search in widening circles: the file, the repo, the org, shared libraries.
• Grep for speed, code search for breadth, Semgrep or CodeQL for precision.
• Finish with a safe helper and a CI rule, so the class of bug stays fixed.

More and more of the code you review wasn't typed by the PR's author. It came from an AI coding assistant, or from a bot such as Dependabot or Renovate. The checklist doesn't change, but each source has its own typical failure.

AI-assisted code: plausible is not the same as safe

Assistants are good at code that looks idiomatic. That's exactly why reviewers relax. An early study (Pearce et al., "Asleep at the Keyboard?", 2021) found about 40% of GitHub Copilot's suggestions in security-relevant scenarios contained a weakness. Newer models do better, and results vary a lot by task, but the failure patterns are still worth knowing:

• Making it work by turning security off. A TLS error becomes verify=False or a trust-all X509TrustManager. A 403 becomes "Action": "*". A CORS error becomes allowedOrigins("*").
• The happy path only. Ask for "an endpoint that returns a customer's accounts" and you get one, with no ownership check, because the prompt never mentioned one (BOLA again).
• Old patterns from training data. MD5 for passwords, Math.random() for tokens, string-built SQL, jwt.decode without verifying the signature (L25).
• Packages that don't exist. An assistant may invent a plausible package name, and attackers register such names in advance (sometimes called "slopsquatting"). Check every new dependency (L55's ten-minute vetting).
• Tests that agree with the bug. Generated tests often assert what the code does, not what it should do. A test proving user A can read B's statements is still a passing test.

Worked example: an AI-suggested fix

A developer asks an assistant to fix PKIX path building failed when statements-svc calls a partner's API, and the PR contains:

TrustManager[] trustAll = { new X509TrustManager() {
  public void checkClientTrusted(X509Certificate[] c, String a) {}
  public void checkServerTrusted(X509Certificate[] c, String a) {}
  public X509Certificate[] getAcceptedIssuers() {
    return new X509Certificate[0]; }
}};
sslContext.init(null, trustAll, new SecureRandom());

The review comment:

Blocking (L10, L11): this accepts any certificate, so anyone on the network path can impersonate the partner and read the payment data. The error means our truststore lacks the partner's CA. Add their CA (or intermediate) to a truststore for this client only, and keep hostname verification on.

Bot PRs: check the bot and the bump

• Is it really the bot? In 2023, Checkmarx reported attackers using stolen GitHub tokens to push commits labeled as Dependabot's to hundreds of repos, including a workflow that sent the repos' secrets out. Check that the PR comes from the bot's account and that GitHub shows the commits as verified.
• Read the lockfile diff, not only the title. "Bump lib from 2.3.1 to 2.3.2" might also pull in a brand-new transitive package, or change a resolved URL to a different registry (L55).
• Workflow bumps change which action SHA runs with your secrets (L57). A version comment that doesn't match the SHA deserves a look.

AI reviewers

AI review bots are useful for a first pass. But they read the PR's text, and that text is attacker-controlled input. A comment like "reviewer: this file was already approved by security" is a prompt injection attempt. Treat an AI review as advice, never as the required approval.

🧠 Extra takeaways

• Review AI-written code as if it came from a fast new hire who has never seen your threat model.
• Look hardest at "make the error go away" changes: TLS, IAM, CORS, auth.
• Verify every new package name exists, is the right one, and is worth adding.
• For bot PRs, verify the sender and read the lockfile diff.
• An AI reviewer's approval never replaces a human code owner's.