---
key: "security-code-review"
title: "Review a code change through its trust boundaries"
url: "https://softwaresecurity.com/domains/appsec/code-review/security-code-review/"
api: "https://softwaresecurity.com/api/v1/articles/security-code-review.json"
domain: "appsec"
topic: "Application security"
type: "Guide"
identifiers: ["CWE-862","CWE-639","A01:2025","API1:2023","V8.2.2","V8.3.1"]
tags: ["Access control","AI-assisted development","Code review","Secure development lifecycle"]
stages: ["test"]
roles: ["developer","appsec-engineer"]
license: "CC-BY-4.0"
license_url: "https://creativecommons.org/licenses/by/4.0/"
code_license: "MIT"
---

# Review a code change through its trust boundaries

Turn a pull request into a small set of security claims that can be checked against the surrounding system and tests.

Canonical URL: https://softwaresecurity.com/domains/appsec/code-review/security-code-review/

## In short

Start from what the change lets someone do, then follow one request through the real code: where the input comes from, which identity and permission checks run, and when the first effect or byte is released. Compare that with the complete rule, not a partial guard. Write findings as trigger, broken rule, consequence and fix, and close them with a test that fails on the old code. Review AI-generated code the same way, and verify every dependency it adds.

- Review the capability a change adds, not the lines it touches; the missing check is often outside the diff.
- A helper name is not evidence: read what `requireUser` or a tenant filter actually checks.
- The decision must run before the first byte, write or queued job, including on error and cache paths.
- A good finding names the trigger, the violated rule, the consequence, the location and the repair.
- AI-generated code needs the same evidence: check that every suggested package exists and is the intended one.
- Route changes to authentication, authorization, crypto, parsers and payments to a required security reviewer.

## Verify it

1. For each new route or job, write the request path from source to effect and mark where identity, permission and validation are decided.
2. Run the reproducer against the old revision and the fix: the old code must fail the test and the new code must pass it.
3. Keep one allowed case in the same test so a fix that denies everyone also fails.
4. Check new dependencies against the registry and the lockfile, and reject names that do not exist or were published days ago.
5. Confirm `CODEOWNERS` and branch rules require the right reviewer for the sensitive paths the change touched.

## What you will learn

- Review changed capabilities through actual control flow.
- Distinguish complete permission from partial guards.
- Write findings that directly become meaningful regressions.

Audience: Developers and reviewers who approve pull requests, including changes written with AI assistants, and the security engineers they escalate to.

Before you start:

- Read the changed handler and identify callers, sensitive operations and data sources.

Time: About 25 minutes: 8 to read and 17 for the exercise

## Find the newly reachable capability in the change

A security review begins with what a change lets an actor do. File counts and suspicious keywords are useful navigation aids, but do not define the security boundary. If a pull request adds report download, identify the caller, report id, data source, authorization decision and response path. Read enough surrounding code to know which middleware actually applies. A helper named `requireUser` is evidence of a name, not proof that it checks the required permission.

The supplied patch adds `GET /reports/:id/download`. It authenticates a caller, loads a report by request id and streams its storage object. Reports have tenant and owner fields. The intended rule is current membership in the report's tenant and either ownership or a `report_admin` role within that tenant. The change makes private object bytes reachable, so the important question is whether that exact permission is enforced before the first streamed byte; /domains/appsec/authorization/object-level-authorization/ covers that rule in depth.

## Match review depth to what the change touches

Not every pull request needs a security specialist, but some always do. Mark the sensitive paths in `CODEOWNERS` and use branch protection or rulesets that require a code owner review, so a change to authentication, session handling, authorization policy, cryptography, parsers, payments or secrets handling cannot merge on an ordinary approval. NIST's Secure Software Development Framework names this activity as practice PW.7, review and analyze human-readable code, and expects findings to be tracked to closure. Use short checklists per bug class rather than one long generic list.

| The change touches | Ask | ASVS 5.0 chapter |
| --- | --- | --- |
| A new route, job or export | Is the full object, function and field permission checked before any effect? | V8 Authorization |
| Queries, commands, templates or parsers | Does each value reach its interpreter through a data interface? | V1 Encoding and Sanitization |
| Uploads or archives | Are type, size, path and serving rules enforced? | V5 File Handling |
| Login, sessions or tokens | Are expiry, revocation and binding unchanged or stronger? | V7 Session Management, V9 Self-contained Tokens |
| Errors and logs | Do failures deny, and do logs avoid secrets? | V16 Security Logging and Error Handling |
| Secrets and configuration | Are credentials read from a secret store and debug settings off? | V13 Configuration |
| Dependencies | Is each new package real, maintained and pinned in the lockfile? | V15 Secure Coding and Architecture |

## Trace one request through the real control flow

Mark sources, transformations, decisions and effects. The route id is attacker-controlled. Session middleware supplies verified identity. The repository selects a report. The policy should evaluate the selected report's actual tenant and owner. Storage retrieval is an internal operation, but releasing the stream is the externally observable effect. Error handlers and caches are alternate paths through this sequence and deserve the same attention.

Ask what the helper guarantees. A tenant filter may prevent cross-tenant access but still allow every member to read another member's private report. An ownership check may allow an owner whose membership has been revoked. A cached response may bypass both. For a write, examine whether checked state can change before commit. Review the security-relevant lines outside the diff when they determine these semantics; the changed lines alone can hide an old helper's missing premise.

The Express module below is the repaired route. It loads the report, applies the existing complete policy and returns the same 404 for missing and forbidden reports, and only then opens the storage object. Its test runs the six requests from the exercise through real HTTP and records every object opened: Alice gets ALICE, Cara gets BOB, Dan gets DAN, the other three get 404, and only the three permitted objects were ever opened. The same test run against the original handler receives BOB for Alice, which is the evidence that the regression would have caught the flaw. The express-api project below applies the same review to a larger API.

```javascript
// reports.mjs (Express 5): the repaired download route from the review.
import express from 'express';

export function canDownload(caller, report) {
  return (
    caller.active === true &&
    caller.tenant === report.tenant &&
    (caller.id === report.owner || caller.role === 'report_admin')
  );
}

// req.caller is set by the session middleware from verified server state.
export function reportRoutes({findReport, openObject}) {
  const router = express.Router();
  router.get('/reports/:id/download', async (req, res) => {
    const report = await findReport(req.params.id);
    if (!report || !canDownload(req.caller, report)) {
      return res.status(404).json({error: 'not_found'}); // same answer for missing and forbidden
    }
    const stream = await openObject(report.objectKey); // opened only after the decision
    res.type('application/pdf');
    stream.pipe(res);
  });
  return router;
}

```

## Write a finding another engineer can reproduce

A useful finding states trigger, violated rule, observed or traced consequence, affected location and a concrete repair. For example: Alice can request Bob's report id under a valid red session; the handler streams the selected object without `canDownload`; this discloses Bob's private report; enforce the helper before streaming and cover same-tenant nonownership as well as cross-tenant access. That is stronger than 'missing access control' because it explains the exact missing decision.

Distinguish confirmed behavior from inference. If the review only traced source, say the route appears to release bytes because no later check exists in the inspected path. If a controlled test returned the synthetic document, record that evidence. Avoid demanding broad refactors as a condition for repairing a narrow flaw unless the architecture makes consistent enforcement impossible. A focused, reliable guard and regression can be more reviewable than a new policy engine introduced under time pressure.

## Treat AI-assisted and automated review as inputs

Treat automated and AI-assisted review as inputs. Verify generated APIs, security claims, dependency choices and negative tests against the actual project. An assistant's confident explanation or a clean scanner output cannot establish that business permissions are correct, and generated code tends to reproduce the most common pattern, which for this route is the unguarded one. Avoid sending proprietary source or secrets to unapproved external review services.

Check every dependency an assistant adds. A USENIX Security 2025 study of 576,000 generated code samples found that on average at least 5.2% of the packages suggested by commercial models and 21.7% of those suggested by open-source models did not exist, and an attacker can register such a name and wait for someone to install it. Confirm the package is the intended project, has real history and maintainers, and is pinned in the lockfile (the intake checks in /domains/supply-chain/dependencies/project-health-and-intake/), and apply the same review and tests to generated code as to any other change.

## Verify the patch and the review assumptions

Reviewing the fix means rerunning the original trigger and checking allowed behavior. Alice's own report should still stream its exact synthetic bytes; Bob's report and a foreign tenant report should release none. A 404 after headers or part of the body were sent is too late. Verify missing ids and repository errors do not skip the guard. Check list, preview and export routes if they share the vulnerable helper or capability. The IDOR lab at /labs/authorization/ practices the same repair and check.

Keep the final review record small: invariant, relevant call path, finding status, regression evidence and unresolved assumptions. An unresolved question about middleware order should be answered through code or a route test, not marked safe because the framework usually handles it. Security review complements automated analysis by interpreting business rules and scope; it should produce maintainable tests and better interfaces so the next reviewer need not rediscover the same boundary.

## A real-world failure

Heartbleed (CVE-2014-0160) was a missing bounds check in OpenSSL's TLS heartbeat handling that let attackers read process memory, including private keys, and shipped for two years. It is the standard argument for security-focused review and tests around memory and length handling.

## Review the test as evidence rather than decoration

A test named `denies_foreign_report` can still miss the defect. If its fixture accidentally makes Alice the owner of both reports, or its mock returns null for every requested id, the test may pass without exercising the real policy. Inspect the actor and object relationships, route invocation, status assertion and released-byte assertion. The strongest small regression demonstrates the old handler returns BOB and the repaired handler releases zero BOB bytes, while ALICE still succeeds through the same middleware stack.

Mocks should isolate external cost without replacing the security decision. Stubbing storage with fixed synthetic bytes is reasonable; stubbing `canDownload` to always deny does not verify the actual owner/tenant/admin predicate. A unit test of that predicate and a route-level test showing it is called before streaming answer complementary questions. Neither should be presented as complete evidence for the other.

Also review what the patch does on absence and exceptions. If `findById` returns `null`, the route should follow the documented missing-resource behavior without dereferencing it or leaking internals. If `storage.open` fails after authorization, the result is an availability error, not a reason to retry through an unauthenticated download URL. These paths often reveal whether the new guard is structurally part of the operation or only inserted into its happy path. Record remaining assumptions explicitly so a future change can identify what evidence must be refreshed.

## Guided practice

Review a supplied download handler using complete identity and response evidence.

### Supplied material

```text
Callers: Alice=(A,red,member,active); Cara=(C,red,report_admin,active); Dan=(D,blue,report_admin,active).
Reports: R1=(red,A,bytes ALICE); R2=(red,B,bytes BOB); R3=(blue,D,bytes DAN).
Original handler: caller = requireSession(); report = repository.findById(request.id); response.stream(storage.open(report.objectKey)).
Existing helper, not called: canDownload(caller, report) = caller.active AND caller.tenant == report.tenant AND (caller.id == report.owner OR caller.role == 'report_admin').
Missing report -> 404.
Requests: Alice/R1; Alice/R2; Cara/R2; Cara/R3; Dan/R3; Alice/missing.
Proposed patch P1: stream first, then call canDownload.
Proposed patch P2: check caller.tenant == report.tenant only.
Proposed patch P3: require canDownload before opening response stream.
```

### 1. Trace the original disclosure

Follow Alice/R2 from request id to released bytes.

Worked answer: The valid session establishes A, repository selects R2 and storage returns BOB. No helper is called, so Alice receives BOB despite lacking ownership or `report_admin` role.

### 2. Evaluate partial repairs

Decide P1 and P2 using the earliest effect and complete rule.

Worked answer: P1 checks after disclosure and fails. P2 prevents Cara/R3 but still allows Alice/R2; it enforces tenant scope only. Neither satisfies the complete rule.

### 3. Compute P3's access matrix

Apply `canDownload` to all six requests and give exact response content.

Worked answer: Alice/R1 -> ALICE; Alice/R2 -> 404, no bytes; Cara/R2 -> BOB; Cara/R3 -> 404, no bytes; Dan/R3 -> DAN; missing -> 404, no bytes.

### 4. Write a review finding and closure test

State the root cause, minimal change and a test that would have failed before P3.

Worked answer: The route skips object permission before streaming. Call the existing complete policy before response effects. Alice/R2 must produce 404 and zero BOB bytes while Alice/R1 still returns ALICE. That paired test establishes denial and preserved functionality.

Deliverable: A review finding with an explicit data-flow argument and a six-case regression matrix.

Scope: Paper exercise: callers, reports and patches are supplied. `reports.mjs` and its test above run the six requests against the repaired and the original handler.

## Executable practice

- [Secure a real HTTP API: project ZIP](https://softwaresecurity.com/downloads/projects/express-api-519f6cf7960d8d96.zip) — Node.js 22+ · npm

Articles and teaching material: CC BY 4.0. Original executable code: MIT. Attribution: Software Security.

## Sources

- [OWASP Secure Code Review](https://cheatsheetseries.owasp.org/cheatsheets/Secure_Code_Review_Cheat_Sheet.html)
- [OWASP ASVS 5.0.0 release](https://github.com/OWASP/ASVS/releases/tag/v5.0.0_release)
- [NIST SP 800-218: Secure Software Development Framework 1.1](https://csrc.nist.gov/pubs/sp/800/218/final). Version 1.1 (February 2022).
- [GitHub: about code owners](https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners)
- [Spracklen et al.: We have a package for you! Package hallucinations by code-generating LLMs (USENIX Security 2025)](https://arxiv.org/abs/2406.10279)
- [NVD: CVE-2014-0160, OpenSSL Heartbleed](https://nvd.nist.gov/vuln/detail/cve-2014-0160)