Secure Code Review Methodology: What to Look For and How to Scale It

SAST tools are excellent at finding well-defined vulnerability patterns: SQL injection via string concatenation, hardcoded credentials, unsafe deserialization. They struggle with vulnerabilities that require understanding business logic and intent.

Vulnerability classes best found by manual review:

1. Business logic flaws

A user can modify their subscription tier by changing a hidden form field. The code passes all SAST checks because there is no injection, no unsafe function -- the flaw is that the server trusts client-provided state.

# SAST sees: safe parameter access
# Reviewer sees: the tier should come from the user's database record, not the request
@app.route('/checkout', methods=['POST'])
def checkout():
    tier = request.form.get('tier')  # attacker sets tier='enterprise'
    price = TIER_PRICES[tier]        # gets enterprise price
    charge_user(price)               # charges enterprise price without verifying entitlement

2. Authentication and authorization logic

IDOR (Insecure Direct Object Reference), broken object-level authorization, privilege escalation through role assumption -- these require understanding which users should have access to which resources.

// SAST sees: normal database query
// Reviewer sees: no verification that the requesting user owns document_id
app.get('/document/:id', authenticate, async (req, res) => {
  const doc = await db.documents.findById(req.params.id);
  // Missing: if (doc.owner !== req.user.id) return res.status(403).send('Forbidden');
  res.json(doc);
});

3. Cryptographic misuse

Using encryption correctly but with wrong parameters -- ECB mode, predictable IVs, key reuse, using encryption when you need authenticated encryption.

4. Race conditions and TOCTOU

Time-of-check to time-of-use vulnerabilities require understanding concurrency and timing -- not detectable by static analysis.

5. Chained vulnerabilities

A low-severity SSRF + an internal metadata API + a cloud credentials endpoint = account takeover. No single finding is critical; the combination is. SAST evaluates each line independently; a reviewer sees the chain.

Effective review rate:

A security reviewer can cover approximately 200 lines of code per hour at a level of detail sufficient to catch logic-level issues. For a 5,000-line PR, that is 25 hours of review -- clearly impractical. Scope security review to the highest-risk code: authentication flows, authorization checks, cryptographic operations, external input handling, and privilege-changing operations.

Use this checklist when reviewing code in each risk category. Focus attention where it matters most.

Authentication and session management:

• Passwords stored with Argon2id, bcrypt, or scrypt (never MD5, SHA-1, or unsalted SHA-256)
• Session tokens generated with CSPRNG (not Math.random() or timestamp)
• Session invalidated on logout (not just client-side cookie deletion)
• Session ID rotated after privilege change (login, role elevation)
• Account lockout after N failed attempts with appropriate reset window
• MFA enrollment enforced for privileged operations, not optional
• Password reset tokens are single-use, time-limited, and sent to verified email only

Authorization:

• Every endpoint checks authorization -- not just authentication
• Object-level authorization: verify the requesting user owns/has access to the specific resource (not just any resource of that type)
• Role checks on every privileged operation -- no client-side role enforcement only
• Vertical privilege escalation: can a regular user perform admin operations by manipulating parameters?
• Horizontal privilege escalation: can user A access user B's data by changing an ID?

Input validation:

• All input validated against allowlist (not denylist)
• File uploads: validate type (magic bytes, not just extension), size, filename (path traversal), content scan
• Redirects validated against an allowlist of allowed destinations
• JSON/XML parsers: XXE disabled, schema validation applied

Injection:

• All database queries parameterized (no string concatenation into SQL)
• ORM used correctly -- raw query methods (raw(), execute()) avoided
• Shell commands avoided; if required, arguments passed as array (not string)
• Template engines: auto-escaping enabled; | safe filter and |raw used only where documented

Cryptography:

• AES-256-GCM (or ChaCha20-Poly1305) for symmetric encryption -- not AES-CBC without MAC, not ECB
• Nonces/IVs are unique per operation (random or counter-based, never constant)
• No hardcoded secrets, keys, tokens, or passwords in source
• External APIs secrets loaded from environment/secrets manager, not config files
• JWT signature validation enforced with explicit algorithm (not alg: none possible)

Error handling:

• Error messages to users are generic (no stack traces, no DB errors, no internal paths)
• All errors logged with correlation ID for server-side investigation
• Exceptions caught at appropriate level -- no uncaught exceptions exposing internals

Automation handles the mechanical vulnerability classes, freeing manual reviewers to focus on logic and context.

Semgrep: rule-as-code for pattern matching

Semgrep matches code patterns with syntax awareness. Its rule language allows writing security rules that understand code structure, not just text patterns.

# Install Semgrep
pip install semgrep

# Run against a repository with OWASP Top 10 rules
semgrep --config p/owasp-top-ten --config p/secrets .

# Run with specific language security rules
semgrep --config p/python-security .    # Python
semgrep --config p/nodejs-security .   # Node.js
semgrep --config p/java-security .     # Java

# Write a custom rule (detect use of MD5 in Python)
# rules/no-md5.yaml:
# rules:
#   - id: no-md5-password-hash
#     patterns:
#       - pattern: hashlib.md5(...)
#     message: MD5 is not safe for password hashing. Use hashlib with sha256 or bcrypt.
#     severity: ERROR
#     languages: [python]

semgrep --config rules/no-md5.yaml .

CodeQL: dataflow analysis for complex vulnerabilities

CodeQL performs full program analysis, tracking data from sources (user input) to sinks (dangerous functions) through the entire codebase. It catches complex injection patterns that span multiple files and functions.

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

# Run the standard security query suite
codeql database analyze myapp-db   python-security-and-quality.qls   --format=sarif-latest   --output=results.sarif

# View results in VS Code with CodeQL extension, or upload to GitHub Code Scanning

Integrating findings into GitHub PRs:

# .github/workflows/security-review.yml
name: Security Code Review
on: [pull_request]
permissions:
  security-events: write
jobs:
  semgrep:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Semgrep Scan
        uses: returntocorp/semgrep-action@v1
        with:
          config: p/security-audit p/owasp-top-ten p/secrets
  codeql:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: github/codeql-action/init@v3
        with:
          languages: python, javascript
      - uses: github/codeql-action/autobuild@v3
      - uses: github/codeql-action/analyze@v3
        with:
          upload: true  # uploads to GitHub Security tab

Findings appear as inline PR comments, blocking merge for configured severity levels.

With limited AppSec capacity, manual review must be triaged to the highest-risk code.

Risk-based scoping criteria:

Review manually when the PR touches:

• Authentication or session management logic
• Authorization checks (permission gates, role verification)
• Cryptographic operations (key generation, encryption, signing)
• External input handling (new API endpoints, file upload handling)
• Database query construction (especially raw/native queries)
• Security-critical configuration (CORS, CSP, security headers)
• Secrets or credential handling
• New third-party library with significant capability (HTTP client, templating, ORM)
• Privilege-changing operations (admin functions, payment operations)

Triage matrix:

What to do when you cannot review everything:

1. Security champions program: Train one developer per team as a security champion who conducts first-pass security review before AppSec engagement. Champions learn the review checklist and escalate ambiguous findings.

2. Tiered review queues: Tier 1 app PRs touching security-sensitive code get same-day AppSec review; Tier 2 gets 2-day turnaround; Tier 3 relies on automated tools plus monthly spot-checks.

3. Focused review sprints: Rather than reviewing every PR, schedule dedicated 2-hour security review sessions per team per sprint -- review the most significant changes from the sprint in depth.

A security champions program trains senior developers to conduct first-pass security review, scaling AppSec capacity without proportionally scaling the AppSec headcount.

Security champion selection:

• One champion per engineering team (6-10 developers typically)
• Select from senior engineers with demonstrated quality mindset -- not junior developers
• Champions volunteer or are nominated; forced participation rarely works
• Time commitment: 2-4 hours per week for review and training

Training curriculum for security champions:

Champion review workflow:

1. Champion receives PR notification for their team
2. Champion checks the PR against the security review checklist
3. Low-risk PRs: champion approves
4. Uncertain findings: champion escalates to AppSec with specific question
5. Confirmed security findings: champion opens security defect in tracker; PR blocked pending fix
6. Monthly: AppSec reviews champion decisions to calibrate accuracy and provide feedback

Metrics for champion program:

• Security defects found by champions vs. post-production findings (champions should catch before production)
• Champion review coverage (% of security-sensitive PRs reviewed by champion)
• Time from champion review to AppSec escalation (lower = champion is confident)
• Champion retention rate (measure program health)