Code Review for Security

Security bugs are cheapest to fix during code review. This lesson covers what to look for, how to triage findings efficiently, and how to build a review culture where security is a normal part of the conversation — not a gate that slows everyone down.

What makes a security-focused review different

A functional code review asks: "Does this work correctly?"

A security-focused review asks: "What happens when the input is wrong, the user is malicious, or the assumption is invalid?"

You do not need to be a security expert to catch most vulnerabilities in review. You need a checklist and the habit of asking "what if?"

The security review checklist

Authentication and session management

• Does the endpoint require authentication? (If it should, is it enforced on the server?)
• Are sessions created, validated, and expired correctly?
• Is the authentication token transmitted securely (HttpOnly, Secure, SameSite)?
• Are password/credential operations rate-limited?

Authorisation

• Is the user authorised to perform this action? (Not just authenticated — authorised.)
• Are object-level permissions checked? (Can User A access User B's resource?)
• Is the authorisation check on the server, not just hidden in the UI?
• If there is a role, userId, or isAdmin parameter: is it ignored in favour of server-side session data?

Input handling

• Is all input validated against a schema (type, length, format, allowed values)?
• Is input used in SQL queries parameterised?
• Is input rendered in HTML escaped or sanitised?
• Is input used in system commands avoided or properly escaped?
• Is input used in file paths sanitised for path traversal (../)?
• Are file uploads validated (type, size, content) and stored outside the web root?

Data exposure

• Does the API response include only the fields the client needs?
• Are sensitive fields (password hashes, internal IDs, tokens) excluded from responses?
• Do error messages avoid revealing internal details (stack traces, SQL errors, file paths)?
• Are debug endpoints, verbose logging, or development features disabled?

Cryptography and secrets

• Are secrets (API keys, passwords, tokens) kept out of source code?
• Are passwords hashed with bcrypt, scrypt, or Argon2 (not MD5/SHA-1)?
• Is TLS enforced for external communication?
• Are cryptographic operations using well-known libraries, not custom implementations?

Dependencies

• Are new dependencies from trusted sources?
• Is the dependency well-maintained (recent commits, active maintainers)?
• Does the dependency have known vulnerabilities?
• Is the dependency scoped minimally (not a kitchen-sink library for one function)?

Patterns to flag immediately

These patterns are almost always security issues:

SQL concatenation

// 🚩 Flag this immediately
const query = `SELECT * FROM users WHERE id = '${userId}'`;

// ✅ Safe alternative
const query = 'SELECT * FROM users WHERE id = $1';
const result = await db.query(query, [userId]);

Dangerous HTML rendering

// 🚩 Flag this — XSS if userInput contains HTML/JavaScript
<div dangerouslySetInnerHTML={{ __html: userInput }} />

// 🚩 Also flag in other frameworks
// Vue: v-html="userInput"
// Angular: [innerHTML]="userInput"

Hardcoded credentials

# 🚩 Flag this — even in "test" code that might reach production
db_password = "super_secret_password_123"
api_key = "sk-live-abc123def456"

Missing authorisation

// 🚩 Flag this — authentication without authorisation
app.delete('/api/users/:id', requireAuth, async (req, res) => {
  await db.deleteUser(req.params.id);  // Anyone authenticated can delete ANY user
  res.json({ success: true });
});

// ✅ Fixed — check ownership or admin role
app.delete('/api/users/:id', requireAuth, async (req, res) => {
  if (req.user.id !== req.params.id && !req.user.isAdmin) {
    return res.status(403).json({ error: 'Forbidden' });
  }
  await db.deleteUser(req.params.id);
  res.json({ success: true });
});

Command injection

# 🚩 Flag this — user input in shell command
import os
os.system(f"convert {user_filename} output.png")

# ✅ Fixed — use subprocess with argument list
import subprocess
subprocess.run(["convert", user_filename, "output.png"], check=True)

Path traversal

// 🚩 Flag this — user controls file path
const filePath = path.join('/uploads', req.params.filename);
// "../../../etc/passwd" resolves outside /uploads

// ✅ Fixed — validate the resolved path
const filePath = path.join('/uploads', req.params.filename);
if (!filePath.startsWith('/uploads/')) {
  return res.status(400).json({ error: 'Invalid path' });
}

Insecure randomness

// 🚩 Flag this — Math.random() is not cryptographically secure
const resetToken = Math.random().toString(36).slice(2);

// ✅ Fixed — use crypto module
import { randomBytes } from 'crypto';
const resetToken = randomBytes(32).toString('hex');

Triaging findings

Not every finding is critical. Prioritise based on:

How to communicate findings

Bad: "This is insecure!!! Fix it!!!"

Good:

## Security: SQL injection in search endpoint

**File:** src/routes/search.ts:42
**Severity:** High — user input is concatenated into a SQL query

**The issue:**
The search term from `req.query.q` is interpolated directly into the SQL string.
An attacker can inject SQL to extract or modify data.

**Example exploit:**
`GET /api/search?q=' UNION SELECT password FROM users--`

**Fix:** Use parameterised queries:
```sql
SELECT * FROM products WHERE name ILIKE $1

Pass req.query.q as a parameter.

Explain the risk, show the exploit, and suggest the fix. Be specific and constructive.

## Automated security checks in review

Supplement manual review with automated tools:

### Pre-commit hooks

```yaml
# .pre-commit-config.yaml
repos:
  - repo: https://github.com/Yelp/detect-secrets
    rev: v1.5.0
    hooks:
      - id: detect-secrets
        args: ['--baseline', '.secrets.baseline']

CI security gates

# Run on every PR
security-review:
  steps:
    - name: SAST scan
      run: semgrep scan --config auto --error

    - name: Dependency audit
      run: npm audit --audit-level=high

    - name: Secret detection
      run: trufflehog git file://. --since-commit HEAD~1 --fail

These catch the obvious issues automatically, freeing human reviewers to focus on logic and design.

Building a security review culture

Make it normal, not special

• Include security items in your standard review checklist, not a separate "security review" process
• Every PR reviewer should check the security checklist, not just a designated "security person"
• Treat security findings the same as bugs — file them, fix them, verify them

Rotate security focus

Designate a "security champion" each sprint who pays extra attention to security in reviews. Rotate the role so everyone builds the skill.

Share findings as learning

When you find a security issue in review, share it (anonymised if needed) in a team channel or standup. Not as blame, but as learning:

"Found an IDOR in the orders API during review today. The endpoint checked auth but not ownership — any logged-in user could view any order. We fixed it by adding an ownership check. Worth looking for similar patterns elsewhere."

Review your own code first

Before requesting review, go through the security checklist yourself. Most issues are caught by the author when they know what to look for.

Summary

Security code review is not about being a security expert — it is about consistently applying a checklist. Flag SQL concatenation, dangerous HTML rendering, missing authorisation, hardcoded credentials, and command injection immediately. Triage findings by exposure, input source, data sensitivity, and exploitability. Communicate findings constructively with specific exploit examples and suggested fixes. Supplement manual review with automated SAST, dependency scanning, and secret detection in CI. Make security a normal part of every review, not a separate gate.