A Practical Guide to Secure Code Review

Secure code review is the systematic examination of source code to identify security vulnerabilities. It complements automated SAST, dynamic testing, and penetration testing. Code review catches what tools miss, in particular authorization logic, business logic flaws, design errors, and language-specific anti-patterns that require human judgment.

Reviews fall on a spectrum. Quick PR reviews focus on the diff in front of you, typically minutes per change. Deep dive reviews examine entire components or services over hours or days, often as part of a major release, audit, or third-party assessment. Threat-modeled reviews follow specific threat agents and assets through the code. Each style has its place.

The OWASP Code Review Guide is the canonical reference, with detailed checklists by language and bug class. The OWASP Top 10, ASVS, and language-specific cheat sheets provide structured prompts to bring to a review.

A good reviewer combines breadth (knowing many vulnerability classes) with depth (understanding the application's specific architecture, dependencies, and threat model). The most effective reviews start from a hypothesis about what could go wrong and look for whether it does.

Define scope before starting. A PR review covers the diff. A component review covers a service or module. A full audit covers an entire codebase. Without explicit scope, reviews drift, take longer than expected, and miss what mattered.

Understand the application. Read the architecture documents, threat models if any, and recent incidents. Map the trust boundaries: where untrusted data enters, where sensitive data lives, where privilege is granted. Most vulnerabilities live near these boundaries.

Build a list of hotspots. Authentication, authorization, session management, input handling, output encoding, cryptography, file handling, network communication, deserialization, command execution, and any custom security control are where bugs concentrate. Focus there before sweeping the rest.

Read the dependencies. Many vulnerabilities come from how an application uses libraries rather than the libraries themselves. Know which version of each major dependency is in use and how the application calls it.

Set up the environment. Clone the repo, get the application building locally if possible, and have your editor configured with go-to-definition, find-references, and language-aware search. Reviewing in a browser-only diff is far less effective than reviewing with code navigation.

Trust boundaries are the first stop. Every place that user input enters the application is a place to verify validation and authorization. Search for HTTP handlers, message queue consumers, file readers, deserializers, and IPC entry points. Trace each input to see what it influences and whether it is correctly checked.

Authentication and session management deserve scrutiny. Look for token issuance and validation, session creation and expiration, password handling, MFA enforcement, account recovery, and OAuth flows. Common bugs include algorithm confusion in JWT, missing signature verification, infinite session lifetimes, password reset bypass, and OAuth scope abuse.

Authorization is where most logic bugs hide. For each endpoint or function, ask: who can call this, on which resources, under what conditions? Check that authorization is enforced server-side, applied to every action, and re-checked on each request. Look for IDOR (insecure direct object reference) patterns: identifiers from requests used to fetch or modify objects without ownership checks.

Cryptography requires careful reading. Look for hard-coded keys, custom crypto implementations, weak algorithms (MD5, SHA-1 for security, DES, ECB mode), missing random number generation, predictable nonces, and crypto used outside its safe usage profile. Defer to library defaults rather than custom constructions whenever possible.

Output handling matters as much as input. Look for raw concatenation into HTML, SQL, shell commands, file paths, URLs, log lines, or LDAP queries. Verify contextual encoding (HTML entity encoding, parameterized queries, shell argument arrays, path normalization).

File handling has many footguns. Look for path traversal in upload and download handlers, unsafe deserialization of file contents, unrestricted file types and sizes, and predictable file names.

External integrations are common bug sources. Look for SSRF in URL fetching, command injection in shell invocation, XML external entity (XXE) in XML parsing, server-side template injection in templating engines.

Injection remains pervasive. SQL injection through string concatenation, NoSQL injection through unvalidated query operators, command injection through shell expansion, log injection through unescaped log writes, and prompt injection in LLM-using code all share the same root: untrusted input flowing into a context where it is interpreted.

Authorization gaps are everywhere. Missing checks (no authorization at all), wrong subject (using a request-provided identifier instead of the authenticated user), wrong scope (checking ownership but not permissions), and wrong context (production access for users in test mode) all appear regularly.

Cryptography misuse is common. ECB mode for block ciphers, missing initialization vectors, weak key derivation, hand-rolled "encryption" using XOR, reusing nonces for AES-GCM, hashing passwords with general-purpose hashes instead of password-specific (bcrypt, scrypt, Argon2). Defer to libraries; review carefully when custom code exists.

Race conditions are subtle but exploitable. TOCTOU (time-of-check to time-of-use) bugs in file handling, in concurrent state updates that bypass per-row checks, and in payment flows that double-spend. Look for read-then-modify patterns without locking.

Improper error handling leaks data and provides attacker oracles. Stack traces in production responses, verbose error messages on login (revealing whether a user exists), and differential timing on authentication or authorization decisions all surface in code review.

Insecure deserialization remains a top vulnerability. Pickle, Java serialization, .NET BinaryFormatter, PHP unserialize, and unsafe YAML loaders can all execute arbitrary code on untrusted input. Look for any path where serialized data crosses a trust boundary.

Insecure defaults and feature flags. Newly added features sometimes default to insecure modes; old features sometimes have insecure defaults that were never revisited. Search for environment-variable-controlled toggles, debug flags, and admin shortcuts.

Use SAST as a starting point, not as the review. Semgrep, CodeQL, SonarQube, Snyk Code, and language-specific linters surface common patterns. The reviewer's job is to interpret findings in context, suppress false positives, escalate true positives, and use the findings as prompts for deeper investigation.

Write custom Semgrep or CodeQL rules for organization-specific patterns. Patterns like "raw SQL string concatenation with user input" or "missing tenant scoping in queries" can be encoded once and checked across the codebase. Custom rules pay off over time.

Use grep aggressively. Search for risky function names (system, exec, eval, deserialize, pickle.load), insecure URL patterns (http://, file://, gopher://), magic strings (TODO security, FIXME auth, allowAll), and known credentials patterns.

Use code search platforms. Sourcegraph, GitHub Code Search, and similar tools enable cross-repository searches that surface patterns at scale. Particularly valuable for organizations with many services.

Use git history. Recent changes to security-sensitive files deserve extra scrutiny. git blame and git log reveal context: who introduced this, when, in what change. Combine with PR descriptions and issue links to understand intent.

The way you communicate findings determines whether they get fixed. A perfect finding ignored helps no one.

Be specific. Reference the exact file, line, and code. Explain the vulnerability, show how it can be exploited, and propose a fix. Bonus points for offering an example diff.

Rank severity honestly. Use a consistent rubric (CVSS, organization-specific severity scale, or a four-tier critical/high/medium/low). Avoid inflating severity to drive urgency; over time it erodes trust.

Educate as you go. Many findings represent gaps in developer knowledge rather than carelessness. A short paragraph explaining why a pattern is dangerous, with links to deeper resources, helps the developer avoid the same mistake elsewhere.

Distinguish must-fix from nice-to-fix. Blocking comments should be for genuine security issues. Style preferences and refactoring suggestions belong elsewhere. Overloading PR reviews with non-security feedback dilutes the security message.

Engage in dialogue. Sometimes a finding is wrong, or there is context you missed. Be open to discussion and revise your assessment when the evidence warrants. A reviewer who admits mistakes builds more trust than one who doubles down.

Close the loop. Track findings to resolution. Confirm fixes are correct. Maintain a list of recurring issues for training, automation, or architectural change.

A common pattern in fintech reviews: endpoints that authenticate the user but use a request-provided account ID without verifying ownership. The fix is a one-line lookup, but the vulnerability has caused multiple disclosed breaches across the industry.

In a healthcare review, a deserialization of patient demographics from a partner API led to remote code execution because the application used Python pickle for cross-system data exchange. The fix was to switch to JSON; the long-term remediation was an organization-wide ban on pickle for cross-trust-boundary data.

A major SaaS application's webhook handler made requests to the configured webhook URL without restricting destination. SSRF allowed reaching internal services, including AWS instance metadata, leading to credential theft. The fix was an allow list of permitted destinations and a fetch library that refused private and link-local addresses.

A startup's authentication middleware checked tokens correctly but a new endpoint introduced via copy-paste skipped the middleware entirely. Code review caught it before deployment. The follow-up was a Semgrep rule that flags endpoint handlers missing the middleware decorator.