Security Review: Conducting Effective Security Code Reviews

In this tutorial, you will learn about Security Review: Conducting Effective Security Code Reviews. We cover key concepts, practical examples, and best practices to help you master this topic.

Security code reviews examine source code for security vulnerabilities that automated tools might miss. While SAST tools catch many issues, manual review is essential for finding business logic flaws, authentication bypasses, and authorization gaps that tools cannot detect.

flowchart LR
    PR[Pull Request] --> AutoScan[Automated Scan]
    AutoScan -->|SAST + Dep Scan| Results[Scan Results]
    Results -->|No Issues| ManualReview[Manual Security Review]
    Results -->|Issues Found| Block[Request Changes]
    ManualReview --> Checklist[Security Review Checklist]
    Checklist --> Reviewer[Security Reviewer]
    Reviewer -->|Approve| Merge[Approve PR]
    Reviewer -->|Issues Found| Changes
    Changes --> PR

What You'll Learn

• Security code review methodology and focus areas
• Vulnerability patterns to look for in Node.js code
• Security review checklists for different feature types
• Integrating security review into the development workflow

Why It Matters

Automated tools catch known vulnerability patterns but miss contextual issues: "is this authorization check in the right place?", "can this business logic be abused?", "is sensitive data exposed in this API response?" Manual review catches what tools miss.

Real-World Use

A security reviewer noticed that a password reset endpoint accepted the reset token in the URL query parameter, which would be logged by the web server and visible in browser history. Moving the token to the request body prevented credential leakage.

Security Review Implementation

Automated Security Review Assistant

class SecurityReviewAssistant {
  constructor() {
    this.patterns = [
      {
        name: 'SQL Injection Risk',
        pattern: /db\.(query|execute)\(.*\$\{/,
        severity: 'CRITICAL',
        message: 'String interpolation in SQL query. Use parameterized queries.'
      },
      {
        name: 'Sensitive Data in Logs',
        pattern: /(console\.log|logger\.info)\(.*(password|token|secret|key)/i,
        severity: 'HIGH',
        message: 'Potential sensitive data logging'
      },
      {
        name: 'Insecure Randomness',
        pattern: /Math\.random\(\)/,
        severity: 'MEDIUM',
        message: 'Math.random() is not cryptographically secure. Use crypto.randomBytes()'
      },
      {
        name: 'Eval Usage',
        pattern: /eval\(/,
        severity: 'CRITICAL',
        message: 'eval() allows arbitrary code execution. Avoid it.'
      },
      {
        name: 'Hardcoded Credentials',
        pattern: /(password|secret|api_key|apikey)\s*[:=]\s*['"][^'"]+['"]/i,
        severity: 'CRITICAL',
        message: 'Hardcoded credential detected. Use environment variables.'
      },
      {
        name: 'No Auth on Route',
        pattern: /router\.(get|post|put|delete)\(['"][^'"]+['"],\s*\(/,
        severity: 'HIGH',
        message: 'Route registered without authentication middleware. Verify auth is intended.'
      },
      {
        name: 'Insecure Comparison',
        pattern: /(password|token|hash)\s*===\s*/i,
        severity: 'MEDIUM',
        message: 'Use timing-safe comparison (crypto.timingSafeEqual) for secrets.'
      }
    ];
  }

  async reviewPR(diffText) {
    const findings = [];

    for (const pattern of this.patterns) {
      const matches = diffText.match(pattern.pattern);
      if (matches) {
        findings.push({
          ...pattern,
          lines: this.findLines(diffText, pattern.pattern)
        });
      }
    }

    // Check for missing authorization decorators
    const newRoutes = this.findNewRoutes(diffText);
    for (const route of newRoutes) {
      if (!this.hasAuthMiddleware(diffText, route)) {
        findings.push({
          name: 'Missing Authorization',
          severity: 'HIGH',
          message: `Route ${route} may be missing authorization middleware`,
          lines: [route.line]
        });
      }
    }

    return findings;
  }

  findLines(text, regex) {
    const lines = text.split('\n');
    return lines
      .map((line, i) => line.match(regex) ? i + 1 : null)
      .filter(Boolean);
  }

  findNewRoutes(diffText) {
    const routePattern = /router\.(get|post|put|delete)\(['"]([^'"]+)['"]/g;
    const routes = [];
    let match;
    while ((match = routePattern.exec(diffText)) !== null) {
      routes.push({ method: match[1], path: match[2], line: this.findLines(diffText, match[0])[0] });
    }
    return routes;
  }

  hasAuthMiddleware(diffText, route) {
    const contextBefore = this.getContextBeforeLine(diffText, route.line, 5);
    return /authenticate|authorize|auth|requireAuth|protect/i.test(contextBefore);
  }

  getContextBeforeLine(text, lineNum, linesBefore) {
    const lines = text.split('\n');
    const start = Math.max(0, lineNum - linesBefore - 1);
    return lines.slice(start, lineNum - 1).join('\n');
  }
}

Expected output:

CRITICAL: SQL Injection Risk at line 42 — string interpolation in SQL query
HIGH: Missing Authorization at line 78 — route /api/users/:id/delete without auth middleware
HIGH: Sensitive Data in Logs at line 15 — logging token variable

Security Review Checklist

const securityChecklist = {
  authentication: [
    'Are passwords hashed with bcrypt/argon2?',
    'Is rate limiting applied to login endpoints?',
    'Are JWT tokens signed with RS256?',
    'Is token expiry enforced (access: 15min, refresh: 7d)?',
    'Is refresh token rotation implemented?',
    'Are httpOnly cookies used for token storage?'
  ],

  authorization: [
    'Are authorization checks performed on every endpoint?',
    'Is resource ownership verified?',
    'Are roles and permissions properly scoped?',
    'Is the principle of least privilege applied?',
    'Are authorization failures logged?'
  ],

  inputValidation: [
    'Is all user input validated (type, length, format)?',
    'Are parameterized queries used for all SQL?',
    'Is output encoded for the correct context (HTML, JSON, URL)?',
    'Are file uploads validated for type and size?',
    'Are redirect URLs validated against an allowlist?'
  ],

  dataProtection: [
    'Is sensitive data encrypted at rest?',
    'Is sensitive data encrypted in transit (TLS 1.3)?',
    'Are encryption keys stored separately from data?',
    'Is PII minimized and anonymized where possible?',
    'Are backups encrypted?'
  ],

  logging: [
    'Are security events logged (auth, authZ, validation)?',
    'Are logs structured (JSON) with correlation IDs?',
    'Are sensitive fields masked in logs?',
    'Are logs stored in append-only, immutable storage?'
  ],

  configuration: [
    'Are secrets stored in environment variables or Vault?',
    'Are debug/development endpoints disabled?',
    'Are CORS origins properly restricted?',
    'Are security headers set (CSP, HSTS, XFO)?',
    'Is the principle of least functionality applied?'
  ]
};

function generateReviewReport(findings) {
  const report = {
    summary: {
      total: findings.length,
      critical: findings.filter(f => f.severity === 'CRITICAL').length,
      high: findings.filter(f => f.severity === 'HIGH').length,
      medium: findings.filter(f => f.severity === 'MEDIUM').length,
      low: findings.filter(f => f.severity === 'LOW').length
    },
    findings: findings.sort((a, b) =>
      ({ CRITICAL: 0, HIGH: 1, MEDIUM: 2, LOW: 3 })[a.severity] -
      ({ CRITICAL: 0, HIGH: 1, MEDIUM: 2, LOW: 3 })[b.severity]
    )
  };

  return report;
}

Expected output:

Security Review Report:
Summary: 8 findings (2 CRITICAL, 3 HIGH, 2 MEDIUM, 1 LOW).
Critical: SQL injection in search endpoint, hardcoded AWS secret key.

Common Mistakes

• Relying only on automated tools — they miss business logic flaws, authorization gaps, and contextual vulnerabilities.
• Reviewing only new code — security vulnerabilities often exist because of how new code interacts with existing code.
• Not having a security review checklist — reviewers miss common patterns without a systematic approach.
• Treating the security review as a gate (blocking merge) rather than a collaboration (improving code).
• Not tracking security review findings — recurring issues indicate a need for developer training.

Practice Questions

1. What is the difference between a security code review and a standard code review?
2. What vulnerability types are best found through manual review vs. automated tools?
3. Why is resource ownership verification important in authorization reviews?
4. What should you look for when reviewing authentication-related code?
5. How do you effectively review diff changes for security issues?

Challenge

Create a security review checklist for a payment processing feature. Define 15 review items covering: payment amount validation, currency handling, idempotency keys, Webhook signature verification, refund authorization, and audit logging. Apply the checklist to a sample PR.

FAQ

Mini Project

Create a security review automation tool for pull requests. The tool should: (1) detect common vulnerability patterns in diffs (SQL Injection, hardcoded secrets, missing auth), (2) check for security checklist items, (3) generate a review report with severity ratings, (4) post findings as PR comments. Test on sample PRs with intentional vulnerabilities.

What's Next

Complete the security module with Security Project — a comprehensive hands-on project.