Review a code change through its trust boundaries

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.

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.

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.

// 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;
}

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 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.

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.

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.

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.

Sources & further reading

The standards, specifications and documentation behind the guidance above. Each is the evidence for a claim on this page; consult it for detail beyond the defaults and versions listed here.

1. OWASP Secure Code Review
2. OWASP ASVS 5.0.0 release
3. NIST SP 800-218: Secure Software Development Framework 1.1. Version 1.1 (February 2022).
4. GitHub: about code owners
5. Spracklen et al.: We have a package for you! Package hallucinations by code-generating LLMs (USENIX Security 2025)
6. NVD: CVE-2014-0160, OpenSSL Heartbleed

Reference information

Tags

Access controlAI-assisted developmentCode reviewSecure development lifecycle

SDLC phase

DevelopmentTesting

Role

DevelopersAppSec engineers

ASVS 5.0.0 requirements

v5.0.0-8.2.2 · v5.0.0-8.3.1

Download this guide as Markdown

Continue exploring