Secure Code Review: Finding Vulnerabilities Before They Ship

Most security bugs are not subtle. They are string concatenation into a SQL query. A missing authorization check on an admin endpoint. An MD5 hash on a password. They exist in the code, visible to anyone who knows what to look for, weeks or months before they become a breach headline.

Secure code review is the practice of reading source code through an attacker's lens — systematically hunting for patterns that lead to exploitable vulnerabilities. Done well, it catches a class of bugs that no automated scanner will ever find: business logic flaws, broken access control, insecure design decisions that look perfectly reasonable in isolation.

This guide covers how to do it systematically, what patterns to look for by vulnerability class, which tools accelerate the process, and how code review fits into a modern DevSecOps pipeline alongside dynamic testing.

Secure Code Review vs. SAST: Not the Same Thing

The first confusion to clear up: secure code review is not SAST. Static Application Security Testing tools like Semgrep, CodeQL, Bandit, and Brakeman are valuable — but they are pattern matchers. They find what their rules cover. They miss context, logic, and intent.

Consider this Python snippet:

# SAST will flag this
query = "SELECT * FROM users WHERE id = " + user_id

# SAST will NOT flag this — but it's equally dangerous if
# user_id comes from an untrusted source three call frames up
cursor.execute(query, (user_id,))  # parameterized, but what's in user_id?

A human reviewer traces data flow. A SAST tool matches syntax. Both are useful; neither replaces the other.

The correct posture: run SAST automatically on every commit as a first filter. Use human review to cover what SAST misses — logic, authorization, trust boundaries, cryptographic design.

Why Code Review Complements Dynamic Testing

Dynamic testing (DAST) operates on the running application. It sends payloads, observes responses, and identifies vulnerabilities from the outside. What it cannot do is read your code.

DAST misses vulnerabilities that produce no observable difference in HTTP responses. A broken access control check that returns HTTP 200 with the wrong user's data looks like a successful request to a scanner unless it knows what "wrong user's data" looks like. A hardcoded AWS secret key in source code produces no HTTP anomaly at all.

Code review catches what DAST cannot see:

• Hardcoded credentials and API keys
• Insecure cryptographic algorithm choices
• Subtle authorization logic bugs (role checks that can be bypassed by state manipulation)
• Dangerous dependency configurations
• Race conditions in concurrent code
• Insecure deserialization paths that require specific payload construction to trigger

DAST catches what code review tends to miss: vulnerabilities that emerge from the interaction of components, configuration issues, server-side parsing differences, and anything that requires a live HTTP conversation to observe. Together, they cover the attack surface from both directions.

A Systematic Review Methodology

Reviewing code randomly is how you miss things. A systematic approach covers entry points, trust boundaries, and sensitive operations in a defined order.

Step 1: Map the Attack Surface

Before reading a line of code, understand the application's architecture. What does it do? Where does untrusted data enter? Where are the sensitive operations?

Start by identifying:

• Entry points: HTTP endpoints, websocket handlers, message queue consumers, file upload handlers, CLI argument parsers
• Trust boundaries: Where does the application trust data it shouldn't? What crosses from user-controlled to privileged context?
• Sensitive operations: Authentication, authorization decisions, cryptographic operations, file system access, subprocess execution, database writes
• External integrations: Third-party APIs, webhooks, OAuth flows, SAML assertions

In a Rails app, start with config/routes.rb. In a Node/Express app, grep for app.get, app.post, router.use. In Django, read urls.py. The router is your map.

Step 2: Follow Data From Source to Sink

Pick an entry point. Trace every piece of user-controlled data from where it enters to where it is used. The classic vulnerability pattern is: tainted input reaches a dangerous sink without adequate validation or escaping.

Common sources (untrusted input):

# Python / Django
request.GET['param']
request.POST['param']
request.body
request.headers['X-Custom-Header']
os.environ.get('USER_INPUT')  # sometimes

# Node.js / Express
req.query.param
req.body.field
req.params.id
req.headers['x-forwarded-for']

# Java / Spring
@RequestParam String input
@PathVariable String id
@RequestBody UserDto body
request.getHeader("X-Real-IP")

Common dangerous sinks:

# SQL execution
cursor.execute(query)
connection.query(sql)
entityManager.createNativeQuery(sql)

# Shell execution
subprocess.run(cmd, shell=True)
exec(user_input)
Runtime.getRuntime().exec(command)

# File system
open(user_path, 'r')
fs.readFileSync(userPath)
new File(userInput)

# Template rendering
render_template_string(user_input)   # Jinja2 SSTI
eval(userInput)                       # JavaScript
Velocity.evaluate(ctx, writer, "log", userInput)  # Java

# Deserialization
pickle.loads(user_data)
ObjectInputStream.readObject()
yaml.load(data)  # without Loader=yaml.SafeLoader

Step 3: Review Authentication and Authorization Separately

Authentication and authorization bugs are the most impactful vulnerabilities in most applications. Review them as dedicated passes, not as you happen to encounter them.

For authentication: find every code path that establishes a session or issues a token. Verify that password comparison is constant-time, that account lockout is enforced, that reset tokens are single-use and expire.

For authorization: for every sensitive endpoint, confirm there is an explicit check that the requesting user has permission to perform the action. Absence of a check is the vulnerability — you are looking for what is not there.

Step 4: Audit Cryptographic Operations

Cryptography is easy to get wrong in ways that are not visible to users or DAST scanners. Grep for every cryptographic primitive in use and verify it is being used correctly.

Step 5: Check Dependency Configuration and Secrets

Scan for hardcoded credentials, review third-party library versions for known CVEs, and check that security-relevant configuration (TLS settings, cookie flags, CORS policies) matches intent.

Vulnerability Patterns by Class

SQL Injection

# VULNERABLE — direct concatenation
def get_user(username):
    query = "SELECT * FROM users WHERE username = '" + username + "'"
    return db.execute(query)

# VULNERABLE — f-string interpolation
query = f"SELECT * FROM orders WHERE user_id = {user_id} AND status = '{status}'"

# VULNERABLE — ORM raw escape hatch
User.objects.raw(f"SELECT * FROM users WHERE name = '{name}'")

# SAFE — parameterized query
def get_user(username):
    return db.execute("SELECT * FROM users WHERE username = %s", (username,))

# SAFE — ORM with parameter binding
User.objects.filter(username=username)

Semgrep rule to catch Python string concatenation into SQL:

# semgrep rule: sql-injection-string-concat.yaml
rules:
  - id: sql-string-concat
    patterns:
      - pattern: |
          $QUERY = "..." + $VAR
          $DB.execute($QUERY)
      - pattern: |
          $DB.execute("..." + $VAR)
      - pattern: |
          $DB.execute(f"...{$VAR}...")
    message: "Potential SQL injection: user-controlled data concatenated into query"
    languages: [python]
    severity: ERROR

For grep-based triage across a large codebase:

# Find raw SQL with string formatting
grep -rn --include="*.py" -E "execute\(['\"]SELECT|execute\(.*%s.*%" .
grep -rn --include="*.js" -E "query\(`SELECT|query\(.*\+" .
grep -rn --include="*.java" -E "createNativeQuery|createQuery.*\+" .

# Find ORM raw escape hatches
grep -rn "\.raw(" .
grep -rn "extra(select\|where" .   # Django ORM extra()
grep -rn "queryBuilder\|nativeQuery" .

Authentication Bypass

# VULNERABLE — type coercion (PHP loose comparison)
if ($token == $stored_token) { /* login */ }
// Attack: token=0 matches any non-numeric stored token ("abc" == 0 is TRUE in PHP)

// SAFE — strict comparison
if ($token === $stored_token) { /* login */ }

# VULNERABLE — JWT algorithm confusion
import jwt
decoded = jwt.decode(token, public_key, algorithms=["RS256", "HS256"])
# Attack: sign with public key as HMAC secret, specify alg: HS256 in header

# SAFE — pin the algorithm
decoded = jwt.decode(token, public_key, algorithms=["RS256"])

# VULNERABLE — timing-sensitive comparison
if password == stored_password:  # character-by-character short-circuit
    return True

# SAFE — constant-time comparison
import hmac
if hmac.compare_digest(password.encode(), stored_password.encode()):
    return True

# VULNERABLE — password reset token in URL, no expiry
token = hashlib.md5(username + timestamp).hexdigest()
reset_url = f"/reset?token={token}&user={username}"
# Problems: MD5 is weak, user param lets attacker reset any account,
# no expiry, token never invalidated after use

# SAFE pattern
token = secrets.token_urlsafe(32)
store_reset_token(user_id, token, expires_at=now() + timedelta(hours=1))

Patterns to grep for:

# JWT algorithm confusion risks
grep -rn "algorithms=\[" . | grep -v "RS256\"\]"   # algorithms list with multiple entries

# Weak token generation
grep -rn -E "md5|sha1" . | grep -i "token\|secret\|password\|reset"

# PHP loose comparisons in auth context
grep -rn " == " . | grep -i "token\|password\|hash\|auth"

# Missing auth decorators (Django/Flask)
grep -rn "def [a-z_]*view\|def [a-z_]*endpoint" . | head -40
# Then check what decorators each view has

Cryptographic Weaknesses

# VULNERABLE — MD5 for password hashing
import hashlib
hashed = hashlib.md5(password.encode()).hexdigest()

# VULNERABLE — SHA1 without salt
hashed = hashlib.sha1(password.encode()).hexdigest()

# SAFE — bcrypt, scrypt, or Argon2
from passlib.hash import argon2
hashed = argon2.hash(password)

# VULNERABLE — ECB mode (leaks patterns)
from Crypto.Cipher import AES
cipher = AES.new(key, AES.MODE_ECB)

# SAFE — AES-GCM (authenticated encryption)
from Crypto.Cipher import AES
cipher = AES.new(key, AES.MODE_GCM)
ciphertext, tag = cipher.encrypt_and_digest(plaintext)

# VULNERABLE — hardcoded key
SECRET_KEY = "hardcoded_secret_1234567890abcdef"
cipher = AES.new(SECRET_KEY.encode(), AES.MODE_CBC, IV=b"1234567890123456")

# VULNERABLE — predictable IV
IV = b"\x00" * 16  # or any static value

# VULNERABLE — weak random for security-sensitive purposes
import random
token = str(random.randint(100000, 999999))  # predictable

# SAFE — cryptographically secure random
import secrets
token = secrets.token_hex(32)

# Grep for weak algorithms
grep -rn -E "md5|sha1\b|DES|RC4|ECB" . --include="*.py" --include="*.js" --include="*.java"
grep -rn "AES.MODE_ECB\|Cipher.getInstance.*ECB" .
grep -rn "SecretKeySpec\|IvParameterSpec" . | head -20  # audit IV sources

# Grep for hardcoded secrets
grep -rn -E "SECRET_KEY\s*=\s*['\"][^'\"]{8,}" .
grep -rn -E "(password|secret|api_key|token)\s*=\s*['\"][^'\"]{6,}" . --include="*.py"
grep -rn "private_key\s*=" . | grep -v "os\.environ\|config\["

# Weak random
grep -rn "random\.randint\|Math\.random\|rand()" . | grep -i "token\|session\|secret\|key"

Access Control Flaws

# VULNERABLE — IDOR: no ownership check
@app.route('/api/invoices/')
@login_required
def get_invoice(invoice_id):
    # authenticated, but does this invoice belong to the requesting user?
    invoice = Invoice.query.get(invoice_id)
    return jsonify(invoice.to_dict())

# SAFE — explicit ownership check
@app.route('/api/invoices/')
@login_required
def get_invoice(invoice_id):
    invoice = Invoice.query.filter_by(
        id=invoice_id,
        user_id=current_user.id  # ownership enforced at query level
    ).first_or_404()
    return jsonify(invoice.to_dict())

# VULNERABLE — mass assignment (Rails)
def user_params
  params.require(:user).permit!  # permits ALL params including :role, :admin
end

# SAFE — explicit allowlist
def user_params
  params.require(:user).permit(:name, :email, :password)
end

# VULNERABLE — mass assignment (Node.js / Mongoose)
const user = await User.findById(req.params.id);
Object.assign(user, req.body);  # req.body could include { role: 'admin' }
await user.save();

# SAFE — explicit field assignment
const { name, email } = req.body;
user.name = name;
user.email = email;
await user.save();

# VULNERABLE — role check client-side or easily bypassed
if (req.body.isAdmin === true) {  # user-supplied field
    grantAdminAccess();
}

# VULNERABLE — path traversal in file serving
@app.route('/files/')
def serve_file(filename):
    return send_from_directory('/app/uploads', filename)
    # Attack: filename = "../../etc/passwd"

# Find endpoints lacking authorization decorators (Flask)
grep -B2 "@app.route\|@blueprint.route" *.py | grep -B1 "def " | grep -v "@login_required\|@permission_required\|@admin_required"

# Find Object.assign with req.body (Node.js mass assignment)
grep -rn "Object\.assign.*req\.body\|\.assign(user, req\." .

# Find Rails permit!
grep -rn "\.permit!" . --include="*.rb"

# Find IDOR-risky patterns (no user_id filter)
grep -rn "\.get(id)\|\.find(id)\|findById(id)" . | grep -v "user_id\|owner\|current_user"

Command Injection

# VULNERABLE — shell=True with user input
import subprocess
subprocess.run(f"ping -c 1 {host}", shell=True)
os.system(f"convert {filename} output.png")

# SAFE — argument list, no shell
subprocess.run(["ping", "-c", "1", host], shell=False)

# VULNERABLE — Node.js exec with user input
const { exec } = require('child_process');
exec(`ffmpeg -i ${userFile} output.mp4`, callback);

# SAFE — execFile (no shell interpolation)
const { execFile } = require('child_process');
execFile('ffmpeg', ['-i', sanitizedFile, 'output.mp4'], callback);

# Grep for shell=True
grep -rn "shell=True" . --include="*.py"
grep -rn "os\.system\|os\.popen\|commands\.getoutput" . --include="*.py"

# Node.js exec patterns
grep -rn "exec(\`\|exec(\"" . --include="*.js" --include="*.ts"
grep -rn "child_process" . | grep "exec\b"  # exec vs execFile

Prioritizing What to Review in Large Codebases

You cannot manually review every line in a 500,000-line codebase. You need a triage strategy that concentrates effort where risk is highest.

Risk-Based Prioritization

Use Git to Focus Effort

# Find the most recently changed files
git log --since="30 days ago" --name-only --pretty=format: | sort | uniq -c | sort -rn | head -20

# Find files changed by the most different authors (high churn / handoff risk)
git log --pretty=format: --name-only | sort | uniq -c | sort -rn | head -30

# Find commits touching auth-related files
git log --all --oneline -- "*auth*" "*login*" "*session*" "*password*" "*token*"

# Find recent changes to security-sensitive patterns
git log -p --since="60 days ago" -S "execute\|eval\|subprocess\|pickle" -- "*.py"

Tools That Accelerate the Process

Semgrep

Semgrep is the most flexible SAST tool for secure code review. Its rule syntax maps closely to code structure, false positive rates are controllable, and you can write custom rules for your codebase's specific patterns in minutes.

# Run the security-audit ruleset
semgrep --config=p/security-audit .

# Run OWASP Top 10 rules
semgrep --config=p/owasp-top-ten .

# Run language-specific security rules
semgrep --config=p/python .
semgrep --config=p/nodejs-security .
semgrep --config=p/java .

# Write a custom rule for your ORM's raw query escape hatch
cat > rules/django-raw-sql.yaml << 'EOF'
rules:
  - id: django-raw-sql
    pattern: $MODEL.objects.raw(...)
    message: "Django raw SQL bypasses ORM protections. Verify parameterization."
    languages: [python]
    severity: WARNING
EOF
semgrep --config=rules/django-raw-sql.yaml .

CodeQL

CodeQL builds a semantic database of your code and lets you query it like a database. It understands data flow across files and call frames — it can find the SQL injection where the tainted value is passed through three function calls before reaching execute(). It is slower to set up but catches what grep and Semgrep miss.

# Initialize CodeQL database for a Python project
codeql database create myapp-db --language=python --source-root=.

# Run the built-in security queries
codeql database analyze myapp-db python-security-extended.qls \
    --format=sarif-latest --output=results.sarif

# Custom query: find all SQL sinks reachable from HTTP handlers
# (saved as find-sqli.ql)
import python
import semmle.python.security.dataflow.SqlInjectionQuery

from SqlInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
where config.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "SQL injection from $@", source.getNode(), "this source"

Bandit (Python)

Bandit is a Python-specific SAST tool that runs fast and integrates easily into CI. It covers common Python security anti-patterns out of the box.

# Basic scan
bandit -r ./myapp

# High severity only, skip test files
bandit -r ./myapp -lll --exclude ./tests

# Output as JSON for pipeline integration
bandit -r ./myapp -f json -o bandit-results.json

# Key checks Bandit runs:
# B101: assert statements in production code (stripped in optimized mode)
# B105/B106/B107: hardcoded password strings
# B201: Flask debug=True
# B301/B302: pickle/marshal deserialization
# B303: MD5/SHA1 for crypto
# B311: random module for security purposes
# B324: hashlib weak hash
# B501-B509: SSL/TLS configuration issues
# B601/B602: subprocess shell injection

Brakeman (Ruby on Rails)

Brakeman is the standard SAST tool for Rails applications. It understands Rails idioms and catches Rails-specific vulnerabilities including mass assignment, SQL injection through ORM methods, and XSS in templates.

# Basic scan
brakeman /path/to/rails/app

# Output as JSON
brakeman -o output.json /path/to/app

# Only high confidence findings
brakeman --confidence-level 2 /path/to/app

# Ignore specific warnings (after triaging false positives)
brakeman --ignore-file brakeman.ignore /path/to/app

# Key checks Brakeman runs:
# SQL injection via find_by_sql, where(string), order(string)
# Mass assignment via attr_accessible / strong_parameters
# XSS via html_safe, raw(), content_tag with user input
# Command injection via system(), exec(), backticks
# Redirect with user-controlled URL

Integrating Code Review Into the DevSecOps Pipeline

Manual code review does not scale to every commit. The goal is to use tooling to eliminate obvious findings automatically, leaving human reviewers focused on what tools cannot assess.

The Layered Model

Layer 1 — Pre-commit hooks: Fast, local, developer-facing. Run in under 30 seconds or developers disable them.

# .pre-commit-config.yaml
repos:
  - repo: https://github.com/PyCQA/bandit
    rev: 1.7.5
    hooks:
      - id: bandit
        args: ["-lll"]  # high severity only, fast
  - repo: https://github.com/returntocorp/semgrep
    rev: v1.45.0
    hooks:
      - id: semgrep
        args: ["--config=p/secrets", "--error"]  # fail on secrets only

Layer 2 — Pull request gate: Broader scan on changed files only. Runs in CI, blocks merge on new findings.

# GitHub Actions — security gate on PRs
name: Security Scan
on: [pull_request]
jobs:
  sast:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Semgrep scan
        run: semgrep --config=p/security-audit --error --sarif > semgrep.sarif
      - uses: github/codeql-action/upload-sarif@v3
        with:
          sarif_file: semgrep.sarif

Layer 3 — Scheduled deep scan: Full CodeQL analysis, dependency audit, secrets scanning. Weekly or on release branches. Results fed to security backlog, not blocked.

Layer 4 — Human review: Focused on high-risk components, new authentication flows, authorization model changes, cryptographic operations. Triggered by component type and change risk, not by line count.

What Triggers a Human Review

Not every PR needs a security-focused human review. Define triggers based on what changes:

• Any change to authentication or session management code
• New endpoints added to the API surface
• Changes to authorization middleware or permission models
• New third-party library integrations
• Any use of cryptographic primitives
• Changes to file upload or file serving logic
• New subprocess or shell execution calls
• Modifications to deserialization code

Use a CODEOWNERS file to enforce this. Security-sensitive paths automatically require a security reviewer's approval before merge.

# .github/CODEOWNERS
# Security-sensitive paths require AppSec review
/src/auth/          @security-team
/src/middleware/    @security-team
**/models/user.*    @security-team
**/crypto/          @security-team
**/uploads/         @security-team

Triage and Tracking

SAST tools will produce false positives. Untriaged noise is as harmful as no scanning — developers learn to ignore the alerts. Build a triage workflow:

1. New findings go into a security backlog, not directly into sprint queues
2. Each finding gets a severity and a false-positive determination within 48 hours
3. Confirmed findings get a risk owner and a remediation SLA based on severity
4. False positives get documented suppression rules to prevent recurrence
5. Metrics track mean time to remediate by severity — not just open finding counts

A Practical Review Checklist

Before closing a security-focused code review, verify:

• Injection: All database queries use parameterized statements or prepared statements. No string concatenation into SQL, shell commands, or LDAP queries.
• Authentication: Password hashing uses bcrypt, scrypt, or Argon2. Token generation uses secrets module or equivalent. JWT algorithm is pinned. Password comparison is constant-time.
• Authorization: Every authenticated endpoint has an explicit authorization check. Object-level queries include ownership filters. No mass assignment vulnerabilities.
• Cryptography: No MD5, SHA1, DES, or RC4. No ECB mode. No static IVs. No hardcoded keys. Keys come from environment or secrets manager.
• Input validation: File uploads validate type by content (not extension), restrict size, and store outside webroot. Redirects validate against an allowlist. Path operations resolve and validate against a base directory.
• Error handling: Exceptions caught at boundaries do not leak stack traces or internal paths to HTTP responses. Error responses are uniform regardless of failure reason (timing-safe).
• Secrets: No credentials, API keys, or private keys committed to source. Configuration reads from environment variables or a secrets manager.
• Dependencies: No known CVEs in direct dependencies above CVSS 7. Pinned versions in lockfiles.